{"thread":{"id":"62815","subject":"[PATCH 00/10] reftable: fix -Wsign-compare warnings","startedAt":"2025-01-16T10:08:42Z","lastAt":"2025-01-20T16:17:34Z","messageCount":28,"participants":["Patrick Steinhardt","Junio C Hamano","Karthik Nayak"],"isPatch":true,"patchVersion":1,"patchTotal":10},"messages":[{"id":"510659","messageId":"20250116-b4-pks-reftable-sign-compare-v1-0-bd30e2ee96e7@pks.im","threadId":"62815","inReplyTo":null,"subject":"[PATCH 00/10] reftable: fix -Wsign-compare warnings","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-16T10:08:32Z","receivedAt":"2025-01-16T10:08:42Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nduring the last steps of converting the reftable codebase to become a\nstandalone library I noticed that the new -Wsign-compare warnings\ncreated a bit of a problem due to the `DISABLE_SIGN_COMPARE_WARNINGS`\nmacro that we started using. As a consequence I wasn't able to easily\ndrop \"git-compat-util.h\" anymore. This patch series is thus addresses\nthe issue by fixing all sign comparison warnings in the reftable\nlibrary.\n\nThanks!\n\nPatrick\n\n---\nPatrick Steinhardt (10):\n meson: stop disabling -Wsign-compare\n reftable/record: drop unused `print` function pointer\n reftable/record: handle overflows when decoding varints\n reftable/basics: adjust `common_prefix_size()` to return `size_t`\n reftable/basics: adjust `hash_size()` to return `uint32_t`\n reftable/block: adapt header and footer size to return a `size_t`\n reftable/block: adjust type of the restart length\n reftable/blocksource: adjust type of the block length\n reftable/blocksource: adjust `read_block()` to return `ssize_t`\n reftable: address trivial -Wsign-compare warnings\n\n meson.build | 1 -\n reftable/basics.c | 10 ++--\n reftable/basics.h | 4 +-\n reftable/block.c | 20 +++----\n reftable/block.h | 14 ++---\n reftable/blocksource.c | 8 +--\n reftable/reader.c | 32 +++++-----\n reftable/reader.h | 6 +-\n reftable/record.c | 116 ++++++++++++++++--------------------\n reftable/record.h | 23 +++----\n reftable/reftable-blocksource.h | 13 ++--\n reftable/reftable-record.h | 4 +-\n reftable/reftable-writer.h | 2 +-\n reftable/stack.c | 12 ++--\n reftable/system.h | 2 -\n reftable/writer.c | 7 +--\n t/unit-tests/t-reftable-basics.c | 2 +-\n t/unit-tests/t-reftable-readwrite.c | 2 +-\n t/unit-tests/t-reftable-record.c | 19 +++++-\n 19 files changed, 148 insertions(+), 149 deletions(-)\n\n\n---\nbase-commit: 757161efcca150a9a96b312d9e780a071e601a03\nchange-id: 20250116-b4-pks-reftable-sign-compare-8eaabae74c06\n\n"},{"id":"510660","messageId":"20250116-b4-pks-reftable-sign-compare-v1-1-bd30e2ee96e7@pks.im","threadId":"62815","inReplyTo":"20250116-b4-pks-reftable-sign-compare-v1-0-bd30e2ee96e7@pks.im","subject":"[PATCH 01/10] meson: stop disabling -Wsign-compare","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-16T10:08:33Z","receivedAt":"2025-01-16T10:08:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In 4f9264b0cd (config.mak.dev: drop `-Wno-sign-compare`, 2024-12-06) we\nhave started an effort to make our codebase compile with -Wsign-compare.\nBut while we removed the -Wno-sign-compare flag from \"config.mak.dev\",\nwe didn't adjust the Meson build instructions in the same way.\n\nFix this.\n\nSigned-off-by: Patrick Steinhardt \n---\n meson.build | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/meson.build b/meson.build\nindex 0064eb64f5..07744c73b1 100644\n--- a/meson.build\n+++ b/meson.build\n@@ -708,7 +708,6 @@ if get_option('warning_level') in ['2','3', 'everything'] and compiler.get_argum\n # These are disabled because we have these all over the place.\n '-Wno-empty-body',\n '-Wno-missing-field-initializers',\n- '-Wno-sign-compare',\n ]\n if compiler.has_argument(cflag)\n libgit_c_args += cflag\n\n-- \n2.48.0.257.gd3603152ad.dirty\n\n"},{"id":"510661","messageId":"20250116-b4-pks-reftable-sign-compare-v1-2-bd30e2ee96e7@pks.im","threadId":"62815","inReplyTo":"20250116-b4-pks-reftable-sign-compare-v1-0-bd30e2ee96e7@pks.im","subject":"[PATCH 02/10] reftable/record: drop unused `print` function pointer","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-16T10:08:34Z","receivedAt":"2025-01-16T10:08:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In 42c424d69d (t/helper: inline printing of reftable records,\n2024-08-22) we stopped using the `print` function of the reftable record\nvtable and instead moved its implementation into the single user of it.\nWe didn't remove the function itself from the vtable though. Drop it.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/record.h | 3 ---\n 1 file changed, 3 deletions(-)\n\ndiff --git a/reftable/record.h b/reftable/record.h\nindex 25aa908c85..a24cb23bd4 100644\n--- a/reftable/record.h\n+++ b/reftable/record.h\n@@ -73,9 +73,6 @@ struct reftable_record_vtable {\n \t * the same type.\n \t */\n \tint (*cmp)(const void *a, const void *b);\n-\n-\t/* Print on stdout, for debugging. */\n-\tvoid (*print)(const void *rec, int hash_size);\n };\n \n /* returns true for recognized block types. Block start with the block type. */\n\n-- \n2.48.0.257.gd3603152ad.dirty\n\n"},{"id":"510662","messageId":"20250116-b4-pks-reftable-sign-compare-v1-3-bd30e2ee96e7@pks.im","threadId":"62815","inReplyTo":"20250116-b4-pks-reftable-sign-compare-v1-0-bd30e2ee96e7@pks.im","subject":"[PATCH 03/10] reftable/record: handle overflows when decoding varints","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-16T10:08:35Z","receivedAt":"2025-01-16T10:08:45Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The logic to decode varints isn't able to detect integer overflows: as\nlong as the buffer still has more data available, and as long as the\ncurrent byte has its 0x80 bit set, we'll continue to add up these values\nto the result. This will eventually cause the `uint64_t` to overflow, at\nwhich point we'll return an invalid result.\n\nRefactor the function so that it is able to detect such overflows. The\nimplementation is basically copied from Git's own `decode_varint()`,\nwhich already knows to handle overflows. The only adjustment is that we\nalso take into account the string view's length in order to not overrun\nit.\n\nWhile at it, refactor `put_var_int()` in the same way by copying over\nthe implementation of `encode_varint()`. While `put_var_int()` doesn't\nhave an issue with overflows, it generates warnings with -Wsign-compare.\nThe implementation of `encode_varint()` doesn't, is battle-tested and at\nthe same time way simpler than what we currently have.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/record.c | 53 +++++++++++++++++-----------------------\n reftable/record.h | 4 +++\n t/unit-tests/t-reftable-record.c | 17 +++++++++++++\n 3 files changed, 44 insertions(+), 30 deletions(-)\n\ndiff --git a/reftable/record.c b/reftable/record.c\nindex 04429d23fe..4e6541c307 100644\n--- a/reftable/record.c\n+++ b/reftable/record.c\n@@ -21,47 +21,40 @@ static void *reftable_record_data(struct reftable_record *rec);\n \n int get_var_int(uint64_t *dest, struct string_view *in)\n {\n-\tint ptr = 0;\n+\tconst unsigned char *buf = in->buf;\n+\tunsigned char c;\n \tuint64_t val;\n \n-\tif (in->len == 0)\n+\tif (!in->len)\n \t\treturn -1;\n-\tval = in->buf[ptr] & 0x7f;\n-\n-\twhile (in->buf[ptr] & 0x80) {\n-\t\tptr++;\n-\t\tif (ptr > in->len) {\n+\tc = *buf++;\n+\tval = c & 0x7f;\n+\n+\twhile (c & 0x80) {\n+\t\tval += 1;\n+\t\tif (!val || (val & (uint64_t)(~0ULL << (64 - 7))))\n+\t\t\treturn -1; /* overflow */\n+\t\tif (buf >= in->buf + in->len)\n \t\t\treturn -1;\n-\t\t}\n-\t\tval = (val + 1) << 7 | (uint64_t)(in->buf[ptr] & 0x7f);\n+\t\tc = *buf++;\n+\t\tval = (val << 7) | (c & 0x7f);\n \t}\n \n \t*dest = val;\n-\treturn ptr + 1;\n+\treturn buf - in->buf;\n }\n \n-int put_var_int(struct string_view *dest, uint64_t val)\n+int put_var_int(struct string_view *dest, uint64_t value)\n {\n-\tuint8_t buf[10] = { 0 };\n-\tint i = 9;\n-\tint n = 0;\n-\tbuf[i] = (uint8_t)(val & 0x7f);\n-\ti--;\n-\twhile (1) {\n-\t\tval >>= 7;\n-\t\tif (!val) {\n-\t\t\tbreak;\n-\t\t}\n-\t\tval--;\n-\t\tbuf[i] = 0x80 | (uint8_t)(val & 0x7f);\n-\t\ti--;\n-\t}\n-\n-\tn = sizeof(buf) - i - 1;\n-\tif (dest->len < n)\n+\tunsigned char varint[10];\n+\tunsigned pos = sizeof(varint) - 1;\n+\tvarint[pos] = value & 127;\n+\twhile (value >>= 7)\n+\t\tvarint[--pos] = 128 | (--value & 127);\n+\tif (dest->len < sizeof(varint) - pos)\n \t\treturn -1;\n-\tmemcpy(dest->buf, &buf[i + 1], n);\n-\treturn n;\n+\tmemcpy(dest->buf, varint + pos, sizeof(varint) - pos);\n+\treturn sizeof(varint) - pos;\n }\n \n int reftable_is_block_type(uint8_t typ)\ndiff --git a/reftable/record.h b/reftable/record.h\nindex a24cb23bd4..721d6c949a 100644\n--- a/reftable/record.h\n+++ b/reftable/record.h\n@@ -34,6 +34,10 @@ static inline void string_view_consume(struct string_view *s, int n)\n \n /* utilities for de/encoding varints */\n \n+/*\n+ * Decode and encode a varint. Returns the number of bytes read/written, or a\n+ * negative value in case encoding/decoding the varint has failed.\n+ */\n int get_var_int(uint64_t *dest, struct string_view *in);\n int put_var_int(struct string_view *dest, uint64_t val);\n \ndiff --git a/t/unit-tests/t-reftable-record.c b/t/unit-tests/t-reftable-record.c\nindex 42bc64cec8..6d912b9c8f 100644\n--- a/t/unit-tests/t-reftable-record.c\n+++ b/t/unit-tests/t-reftable-record.c\n@@ -58,6 +58,22 @@ static void t_varint_roundtrip(void)\n \t}\n }\n \n+static void t_varint_overflow(void)\n+{\n+\tunsigned char buf[] = {\n+\t\t0xFF, 0xFF, 0xFF, 0xFF,\n+\t\t0xFF, 0xFF, 0xFF, 0xFF,\n+\t\t0xFF, 0x00,\n+\t};\n+\tstruct string_view view = {\n+\t\t.buf = buf,\n+\t\t.len = sizeof(buf),\n+\t};\n+\tuint64_t value;\n+\tint err = get_var_int(&value, &view);\n+\tcheck_int(err, ==, -1);\n+}\n+\n static void set_hash(uint8_t *h, int j)\n {\n \tfor (int i = 0; i < hash_size(REFTABLE_HASH_SHA1); i++)\n@@ -544,6 +560,7 @@ int cmd_main(int argc UNUSED, const char *argv[] UNUSED)\n \tTEST(t_reftable_log_record_roundtrip(), \"record operations work on log record\");\n \tTEST(t_reftable_ref_record_roundtrip(), \"record operations work on ref record\");\n \tTEST(t_varint_roundtrip(), \"put_var_int and get_var_int work\");\n+\tTEST(t_varint_overflow(), \"get_var_int notices an integer overflow\");\n \tTEST(t_key_roundtrip(), \"reftable_encode_key and reftable_decode_key work\");\n \tTEST(t_reftable_obj_record_roundtrip(), \"record operations work on obj record\");\n \tTEST(t_reftable_index_record_roundtrip(), \"record operations work on index record\");\n\n-- \n2.48.0.257.gd3603152ad.dirty\n\n"},{"id":"510663","messageId":"20250116-b4-pks-reftable-sign-compare-v1-4-bd30e2ee96e7@pks.im","threadId":"62815","inReplyTo":"20250116-b4-pks-reftable-sign-compare-v1-0-bd30e2ee96e7@pks.im","subject":"[PATCH 04/10] reftable/basics: adjust `common_prefix_size()` to return `size_t`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-16T10:08:36Z","receivedAt":"2025-01-16T10:08:45Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `common_prefix_size()` function computes the length of the common\nprefix between two buffers. As such its return value will always be an\nunsigned integer, as the length cannot be negative. Regardless of that,\nthe function returns a signed integer, which is nonsensical and causes a\ncouple of -Wsign-compare warnings all over the place.\n\nAdjust the function to return a `size_t` instead.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/basics.c | 8 +++-----\n reftable/basics.h | 2 +-\n reftable/record.c | 4 ++--\n reftable/writer.c | 5 ++---\n t/unit-tests/t-reftable-basics.c | 2 +-\n 5 files changed, 9 insertions(+), 12 deletions(-)\n\ndiff --git a/reftable/basics.c b/reftable/basics.c\nindex fe2b83ff83..10b234ea55 100644\n--- a/reftable/basics.c\n+++ b/reftable/basics.c\n@@ -263,14 +263,12 @@ int names_equal(const char **a, const char **b)\n \treturn a[i] == b[i];\n }\n \n-int common_prefix_size(struct reftable_buf *a, struct reftable_buf *b)\n+size_t common_prefix_size(struct reftable_buf *a, struct reftable_buf *b)\n {\n-\tint p = 0;\n-\tfor (; p < a->len && p < b->len; p++) {\n+\tsize_t p = 0;\n+\tfor (; p < a->len && p < b->len; p++)\n \t\tif (a->buf[p] != b->buf[p])\n \t\t\tbreak;\n-\t}\n-\n \treturn p;\n }\n \ndiff --git a/reftable/basics.h b/reftable/basics.h\nindex 4bf71b0954..9ff81a68f8 100644\n--- a/reftable/basics.h\n+++ b/reftable/basics.h\n@@ -169,7 +169,7 @@ static inline void *reftable_alloc_grow(void *p, size_t nelem, size_t elsize,\n #endif\n \n /* Find the longest shared prefix size of `a` and `b` */\n-int common_prefix_size(struct reftable_buf *a, struct reftable_buf *b);\n+size_t common_prefix_size(struct reftable_buf *a, struct reftable_buf *b);\n \n int hash_size(enum reftable_hash id);\n \ndiff --git a/reftable/record.c b/reftable/record.c\nindex 4e6541c307..69c09ec163 100644\n--- a/reftable/record.c\n+++ b/reftable/record.c\n@@ -135,9 +135,9 @@ int reftable_encode_key(int *restart, struct string_view dest,\n \t\t\tuint8_t extra)\n {\n \tstruct string_view start = dest;\n-\tint prefix_len = common_prefix_size(&prev_key, &key);\n+\tsize_t prefix_len = common_prefix_size(&prev_key, &key);\n \tuint64_t suffix_len = key.len - prefix_len;\n-\tint n = put_var_int(&dest, (uint64_t)prefix_len);\n+\tint n = put_var_int(&dest, prefix_len);\n \tif (n < 0)\n \t\treturn -1;\n \tstring_view_consume(&dest, n);\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex 740c98038e..4e6ca2e368 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -585,10 +585,9 @@ static void update_common(void *void_arg, void *key)\n \tstruct common_prefix_arg *arg = void_arg;\n \tstruct obj_index_tree_node *entry = key;\n \tif (arg->last) {\n-\t\tint n = common_prefix_size(&entry->hash, arg->last);\n-\t\tif (n > arg->max) {\n+\t\tsize_t n = common_prefix_size(&entry->hash, arg->last);\n+\t\tif (n > arg->max)\n \t\t\targ->max = n;\n-\t\t}\n \t}\n \targ->last = &entry->hash;\n }\ndiff --git a/t/unit-tests/t-reftable-basics.c b/t/unit-tests/t-reftable-basics.c\nindex 1d640b280f..9ba7eb05ad 100644\n--- a/t/unit-tests/t-reftable-basics.c\n+++ b/t/unit-tests/t-reftable-basics.c\n@@ -120,7 +120,7 @@ int cmd_main(int argc UNUSED, const char *argv[] UNUSED)\n \t\tfor (size_t i = 0; i < ARRAY_SIZE(cases); i++) {\n \t\t\tcheck(!reftable_buf_addstr(&a, cases[i].a));\n \t\t\tcheck(!reftable_buf_addstr(&b, cases[i].b));\n-\t\t\tcheck_int(common_prefix_size(&a, &b), ==, cases[i].want);\n+\t\t\tcheck_uint(common_prefix_size(&a, &b), ==, cases[i].want);\n \t\t\treftable_buf_reset(&a);\n \t\t\treftable_buf_reset(&b);\n \t\t}\n\n-- \n2.48.0.257.gd3603152ad.dirty\n\n"},{"id":"510664","messageId":"20250116-b4-pks-reftable-sign-compare-v1-5-bd30e2ee96e7@pks.im","threadId":"62815","inReplyTo":"20250116-b4-pks-reftable-sign-compare-v1-0-bd30e2ee96e7@pks.im","subject":"[PATCH 05/10] reftable/basics: adjust `hash_size()` to return `uint32_t`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-16T10:08:37Z","receivedAt":"2025-01-16T10:08:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `hash_size()` function returns the number of bytes used by the hash\nfunction. Weirdly enough though, it returns a signed integer for its\nsize even though the size obviously cannot ever be negative. The only\ncase where it could be negative is if the function returned an error\nwhen asked for an unknown hash, but we assert(3p) instead.\n\nAdjust the type of `hash_size()` to be `uint32_t` and adapt all places\nthat use signed integers for the hash size to follow suit. This also\nallows us to get rid of a couple asserts that we had which verified that\nthe size was indeed positive, which further stresses the point that this\nrefactoring makes sense.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/basics.c | 2 +-\n reftable/basics.h | 2 +-\n reftable/block.c | 4 ++--\n reftable/block.h | 10 ++++----\n reftable/reader.c | 2 +-\n reftable/record.c | 52 ++++++++++++++++++----------------------\n reftable/record.h | 16 ++++++-------\n reftable/reftable-record.h | 4 ++--\n t/unit-tests/t-reftable-record.c | 2 +-\n 9 files changed, 44 insertions(+), 50 deletions(-)\n\ndiff --git a/reftable/basics.c b/reftable/basics.c\nindex 10b234ea55..3b5ea27bbd 100644\n--- a/reftable/basics.c\n+++ b/reftable/basics.c\n@@ -272,7 +272,7 @@ size_t common_prefix_size(struct reftable_buf *a, struct reftable_buf *b)\n \treturn p;\n }\n \n-int hash_size(enum reftable_hash id)\n+uint32_t hash_size(enum reftable_hash id)\n {\n \tif (!id)\n \t\treturn REFTABLE_HASH_SIZE_SHA1;\ndiff --git a/reftable/basics.h b/reftable/basics.h\nindex 9ff81a68f8..a2a010a0e1 100644\n--- a/reftable/basics.h\n+++ b/reftable/basics.h\n@@ -171,7 +171,7 @@ static inline void *reftable_alloc_grow(void *p, size_t nelem, size_t elsize,\n /* Find the longest shared prefix size of `a` and `b` */\n size_t common_prefix_size(struct reftable_buf *a, struct reftable_buf *b);\n \n-int hash_size(enum reftable_hash id);\n+uint32_t hash_size(enum reftable_hash id);\n \n /*\n * Format IDs that identify the hash function used by a reftable. Note that\ndiff --git a/reftable/block.c b/reftable/block.c\nindex 9858bbc7c5..2380aabb2f 100644\n--- a/reftable/block.c\n+++ b/reftable/block.c\n@@ -72,7 +72,7 @@ static int block_writer_register_restart(struct block_writer *w, int n,\n }\n \n int block_writer_init(struct block_writer *bw, uint8_t typ, uint8_t *block,\n-\t\t uint32_t block_size, uint32_t header_off, int hash_size)\n+\t\t uint32_t block_size, uint32_t header_off, uint32_t hash_size)\n {\n \tbw->block = block;\n \tbw->hash_size = hash_size;\n@@ -214,7 +214,7 @@ int block_writer_finish(struct block_writer *w)\n \n int block_reader_init(struct block_reader *br, struct reftable_block *block,\n \t\t uint32_t header_off, uint32_t table_block_size,\n-\t\t int hash_size)\n+\t\t uint32_t hash_size)\n {\n \tuint32_t full_block_size = table_block_size;\n \tuint8_t typ = block->data[header_off];\ndiff --git a/reftable/block.h b/reftable/block.h\nindex 0431e8591f..5f67ed74c5 100644\n--- a/reftable/block.h\n+++ b/reftable/block.h\n@@ -30,7 +30,7 @@ struct block_writer {\n \n \t/* How often to restart keys. */\n \tuint16_t restart_interval;\n-\tint hash_size;\n+\tuint32_t hash_size;\n \n \t/* Offset of next uint8_t to write. */\n \tuint32_t next;\n@@ -48,7 +48,7 @@ struct block_writer {\n * initializes the blockwriter to write `typ` entries, using `block` as temporary\n * storage. `block` is not owned by the block_writer. */\n int block_writer_init(struct block_writer *bw, uint8_t typ, uint8_t *block,\n-\t\t uint32_t block_size, uint32_t header_off, int hash_size);\n+\t\t uint32_t block_size, uint32_t header_off, uint32_t hash_size);\n \n /* returns the block type (eg. 'r' for ref records. */\n uint8_t block_writer_type(struct block_writer *bw);\n@@ -72,7 +72,7 @@ struct block_reader {\n \n \t/* the memory block */\n \tstruct reftable_block block;\n-\tint hash_size;\n+\tuint32_t hash_size;\n \n \t/* Uncompressed data for log entries. */\n \tz_stream *zstream;\n@@ -92,7 +92,7 @@ struct block_reader {\n /* initializes a block reader. */\n int block_reader_init(struct block_reader *br, struct reftable_block *bl,\n \t\t uint32_t header_off, uint32_t table_block_size,\n-\t\t int hash_size);\n+\t\t uint32_t hash_size);\n \n void block_reader_release(struct block_reader *br);\n \n@@ -108,7 +108,7 @@ struct block_iter {\n \tuint32_t next_off;\n \tconst unsigned char *block;\n \tsize_t block_len;\n-\tint hash_size;\n+\tuint32_t hash_size;\n \n \t/* key for last entry we read. */\n \tstruct reftable_buf last_key;\ndiff --git a/reftable/reader.c b/reftable/reader.c\nindex ea82955c9b..9df8a5ecb1 100644\n--- a/reftable/reader.c\n+++ b/reftable/reader.c\n@@ -750,7 +750,7 @@ static int reftable_reader_refs_for_unindexed(struct reftable_reader *r,\n \tstruct table_iter *ti;\n \tstruct filtering_ref_iterator *filter = NULL;\n \tstruct filtering_ref_iterator empty = FILTERING_REF_ITERATOR_INIT;\n-\tint oid_len = hash_size(r->hash_id);\n+\tuint32_t oid_len = hash_size(r->hash_id);\n \tint err;\n \n \tREFTABLE_ALLOC_ARRAY(ti, 1);\ndiff --git a/reftable/record.c b/reftable/record.c\nindex 69c09ec163..0ce294078b 100644\n--- a/reftable/record.c\n+++ b/reftable/record.c\n@@ -220,7 +220,7 @@ static int reftable_ref_record_key(const void *r, struct reftable_buf *dest)\n }\n \n static int reftable_ref_record_copy_from(void *rec, const void *src_rec,\n-\t\t\t\t\t int hash_size)\n+\t\t\t\t\t uint32_t hash_size)\n {\n \tstruct reftable_ref_record *ref = rec;\n \tconst struct reftable_ref_record *src = src_rec;\n@@ -228,8 +228,6 @@ static int reftable_ref_record_copy_from(void *rec, const void *src_rec,\n \tsize_t refname_cap = 0;\n \tint err;\n \n-\tassert(hash_size > 0);\n-\n \tSWAP(refname, ref->refname);\n \tSWAP(refname_cap, ref->refname_cap);\n \treftable_ref_record_release(ref);\n@@ -310,13 +308,12 @@ static uint8_t reftable_ref_record_val_type(const void *rec)\n }\n \n static int reftable_ref_record_encode(const void *rec, struct string_view s,\n-\t\t\t\t int hash_size)\n+\t\t\t\t uint32_t hash_size)\n {\n \tconst struct reftable_ref_record *r =\n \t\t(const struct reftable_ref_record *)rec;\n \tstruct string_view start = s;\n \tint n = put_var_int(&s, r->update_index);\n-\tassert(hash_size > 0);\n \tif (n < 0)\n \t\treturn -1;\n \tstring_view_consume(&s, n);\n@@ -356,7 +353,7 @@ static int reftable_ref_record_encode(const void *rec, struct string_view s,\n \n static int reftable_ref_record_decode(void *rec, struct reftable_buf key,\n \t\t\t\t uint8_t val_type, struct string_view in,\n-\t\t\t\t int hash_size, struct reftable_buf *scratch)\n+\t\t\t\t uint32_t hash_size, struct reftable_buf *scratch)\n {\n \tstruct reftable_ref_record *r = rec;\n \tstruct string_view start = in;\n@@ -365,8 +362,6 @@ static int reftable_ref_record_decode(void *rec, struct reftable_buf key,\n \tsize_t refname_cap = 0;\n \tint n, err;\n \n-\tassert(hash_size > 0);\n-\n \tn = get_var_int(&update_index, &in);\n \tif (n < 0)\n \t\treturn n;\n@@ -442,7 +437,7 @@ static int reftable_ref_record_is_deletion_void(const void *p)\n }\n \n static int reftable_ref_record_equal_void(const void *a,\n-\t\t\t\t\t const void *b, int hash_size)\n+\t\t\t\t\t const void *b, uint32_t hash_size)\n {\n \tstruct reftable_ref_record *ra = (struct reftable_ref_record *) a;\n \tstruct reftable_ref_record *rb = (struct reftable_ref_record *) b;\n@@ -486,7 +481,7 @@ static void reftable_obj_record_release(void *rec)\n }\n \n static int reftable_obj_record_copy_from(void *rec, const void *src_rec,\n-\t\t\t\t\t int hash_size UNUSED)\n+\t\t\t\t\t uint32_t hash_size UNUSED)\n {\n \tstruct reftable_obj_record *obj = rec;\n \tconst struct reftable_obj_record *src = src_rec;\n@@ -518,7 +513,7 @@ static uint8_t reftable_obj_record_val_type(const void *rec)\n }\n \n static int reftable_obj_record_encode(const void *rec, struct string_view s,\n-\t\t\t\t int hash_size UNUSED)\n+\t\t\t\t uint32_t hash_size UNUSED)\n {\n \tconst struct reftable_obj_record *r = rec;\n \tstruct string_view start = s;\n@@ -553,7 +548,7 @@ static int reftable_obj_record_encode(const void *rec, struct string_view s,\n \n static int reftable_obj_record_decode(void *rec, struct reftable_buf key,\n \t\t\t\t uint8_t val_type, struct string_view in,\n-\t\t\t\t int hash_size UNUSED,\n+\t\t\t\t uint32_t hash_size UNUSED,\n \t\t\t\t struct reftable_buf *scratch UNUSED)\n {\n \tstruct string_view start = in;\n@@ -617,7 +612,7 @@ static int not_a_deletion(const void *p UNUSED)\n }\n \n static int reftable_obj_record_equal_void(const void *a, const void *b,\n-\t\t\t\t\t int hash_size UNUSED)\n+\t\t\t\t\t uint32_t hash_size UNUSED)\n {\n \tstruct reftable_obj_record *ra = (struct reftable_obj_record *) a;\n \tstruct reftable_obj_record *rb = (struct reftable_obj_record *) b;\n@@ -692,7 +687,7 @@ static int reftable_log_record_key(const void *r, struct reftable_buf *dest)\n }\n \n static int reftable_log_record_copy_from(void *rec, const void *src_rec,\n-\t\t\t\t\t int hash_size)\n+\t\t\t\t\t uint32_t hash_size)\n {\n \tstruct reftable_log_record *dst = rec;\n \tconst struct reftable_log_record *src =\n@@ -773,7 +768,7 @@ static uint8_t reftable_log_record_val_type(const void *rec)\n }\n \n static int reftable_log_record_encode(const void *rec, struct string_view s,\n-\t\t\t\t int hash_size)\n+\t\t\t\t uint32_t hash_size)\n {\n \tconst struct reftable_log_record *r = rec;\n \tstruct string_view start = s;\n@@ -821,7 +816,7 @@ static int reftable_log_record_encode(const void *rec, struct string_view s,\n \n static int reftable_log_record_decode(void *rec, struct reftable_buf key,\n \t\t\t\t uint8_t val_type, struct string_view in,\n-\t\t\t\t int hash_size, struct reftable_buf *scratch)\n+\t\t\t\t uint32_t hash_size, struct reftable_buf *scratch)\n {\n \tstruct string_view start = in;\n \tstruct reftable_log_record *r = rec;\n@@ -969,7 +964,7 @@ static int null_streq(const char *a, const char *b)\n }\n \n static int reftable_log_record_equal_void(const void *a,\n-\t\t\t\t\t const void *b, int hash_size)\n+\t\t\t\t\t const void *b, uint32_t hash_size)\n {\n \treturn reftable_log_record_equal((struct reftable_log_record *) a,\n \t\t\t\t\t (struct reftable_log_record *) b,\n@@ -993,7 +988,7 @@ static int reftable_log_record_cmp_void(const void *_a, const void *_b)\n }\n \n int reftable_log_record_equal(const struct reftable_log_record *a,\n-\t\t\t const struct reftable_log_record *b, int hash_size)\n+\t\t\t const struct reftable_log_record *b, uint32_t hash_size)\n {\n \tif (!(null_streq(a->refname, b->refname) &&\n \t a->update_index == b->update_index &&\n@@ -1047,7 +1042,7 @@ static int reftable_index_record_key(const void *r, struct reftable_buf *dest)\n }\n \n static int reftable_index_record_copy_from(void *rec, const void *src_rec,\n-\t\t\t\t\t int hash_size UNUSED)\n+\t\t\t\t\t uint32_t hash_size UNUSED)\n {\n \tstruct reftable_index_record *dst = rec;\n \tconst struct reftable_index_record *src = src_rec;\n@@ -1074,7 +1069,7 @@ static uint8_t reftable_index_record_val_type(const void *rec UNUSED)\n }\n \n static int reftable_index_record_encode(const void *rec, struct string_view out,\n-\t\t\t\t\tint hash_size UNUSED)\n+\t\t\t\t\tuint32_t hash_size UNUSED)\n {\n \tconst struct reftable_index_record *r =\n \t\t(const struct reftable_index_record *)rec;\n@@ -1092,7 +1087,7 @@ static int reftable_index_record_encode(const void *rec, struct string_view out,\n static int reftable_index_record_decode(void *rec, struct reftable_buf key,\n \t\t\t\t\tuint8_t val_type UNUSED,\n \t\t\t\t\tstruct string_view in,\n-\t\t\t\t\tint hash_size UNUSED,\n+\t\t\t\t\tuint32_t hash_size UNUSED,\n \t\t\t\t\tstruct reftable_buf *scratch UNUSED)\n {\n \tstruct string_view start = in;\n@@ -1113,7 +1108,7 @@ static int reftable_index_record_decode(void *rec, struct reftable_buf key,\n }\n \n static int reftable_index_record_equal(const void *a, const void *b,\n-\t\t\t\t int hash_size UNUSED)\n+\t\t\t\t uint32_t hash_size UNUSED)\n {\n \tstruct reftable_index_record *ia = (struct reftable_index_record *) a;\n \tstruct reftable_index_record *ib = (struct reftable_index_record *) b;\n@@ -1147,14 +1142,14 @@ int reftable_record_key(struct reftable_record *rec, struct reftable_buf *dest)\n }\n \n int reftable_record_encode(struct reftable_record *rec, struct string_view dest,\n-\t\t\t int hash_size)\n+\t\t\t uint32_t hash_size)\n {\n \treturn reftable_record_vtable(rec)->encode(reftable_record_data(rec),\n \t\t\t\t\t\t dest, hash_size);\n }\n \n int reftable_record_copy_from(struct reftable_record *rec,\n-\t\t\t struct reftable_record *src, int hash_size)\n+\t\t\t struct reftable_record *src, uint32_t hash_size)\n {\n \tassert(src->type == rec->type);\n \n@@ -1169,7 +1164,7 @@ uint8_t reftable_record_val_type(struct reftable_record *rec)\n }\n \n int reftable_record_decode(struct reftable_record *rec, struct reftable_buf key,\n-\t\t\t uint8_t extra, struct string_view src, int hash_size,\n+\t\t\t uint8_t extra, struct string_view src, uint32_t hash_size,\n \t\t\t struct reftable_buf *scratch)\n {\n \treturn reftable_record_vtable(rec)->decode(reftable_record_data(rec),\n@@ -1196,7 +1191,7 @@ int reftable_record_cmp(struct reftable_record *a, struct reftable_record *b)\n \t\treftable_record_data(a), reftable_record_data(b));\n }\n \n-int reftable_record_equal(struct reftable_record *a, struct reftable_record *b, int hash_size)\n+int reftable_record_equal(struct reftable_record *a, struct reftable_record *b, uint32_t hash_size)\n {\n \tif (a->type != b->type)\n \t\treturn 0;\n@@ -1204,7 +1199,7 @@ int reftable_record_equal(struct reftable_record *a, struct reftable_record *b,\n \t\treftable_record_data(a), reftable_record_data(b), hash_size);\n }\n \n-static int hash_equal(const unsigned char *a, const unsigned char *b, int hash_size)\n+static int hash_equal(const unsigned char *a, const unsigned char *b, uint32_t hash_size)\n {\n \tif (a && b)\n \t\treturn !memcmp(a, b, hash_size);\n@@ -1213,9 +1208,8 @@ static int hash_equal(const unsigned char *a, const unsigned char *b, int hash_s\n }\n \n int reftable_ref_record_equal(const struct reftable_ref_record *a,\n-\t\t\t const struct reftable_ref_record *b, int hash_size)\n+\t\t\t const struct reftable_ref_record *b, uint32_t hash_size)\n {\n-\tassert(hash_size > 0);\n \tif (!null_streq(a->refname, b->refname))\n \t\treturn 0;\n \ndiff --git a/reftable/record.h b/reftable/record.h\nindex 721d6c949a..137fff3cad 100644\n--- a/reftable/record.h\n+++ b/reftable/record.h\n@@ -49,18 +49,18 @@ struct reftable_record_vtable {\n \t/* The record type of ('r' for ref). */\n \tuint8_t type;\n \n-\tint (*copy_from)(void *dest, const void *src, int hash_size);\n+\tint (*copy_from)(void *dest, const void *src, uint32_t hash_size);\n \n \t/* a value of [0..7], indicating record subvariants (eg. ref vs. symref\n \t * vs ref deletion) */\n \tuint8_t (*val_type)(const void *rec);\n \n \t/* encodes rec into dest, returning how much space was used. */\n-\tint (*encode)(const void *rec, struct string_view dest, int hash_size);\n+\tint (*encode)(const void *rec, struct string_view dest, uint32_t hash_size);\n \n \t/* decode data from `src` into the record. */\n \tint (*decode)(void *rec, struct reftable_buf key, uint8_t extra,\n-\t\t struct string_view src, int hash_size,\n+\t\t struct string_view src, uint32_t hash_size,\n \t\t struct reftable_buf *scratch);\n \n \t/* deallocate and null the record. */\n@@ -70,7 +70,7 @@ struct reftable_record_vtable {\n \tint (*is_deletion)(const void *rec);\n \n \t/* Are two records equal? This assumes they have the same type. Returns 0 for non-equal. */\n-\tint (*equal)(const void *a, const void *b, int hash_size);\n+\tint (*equal)(const void *a, const void *b, uint32_t hash_size);\n \n \t/*\n \t * Compare keys of two records with each other. The records must have\n@@ -137,16 +137,16 @@ void reftable_record_init(struct reftable_record *rec, uint8_t typ);\n \n /* see struct record_vtable */\n int reftable_record_cmp(struct reftable_record *a, struct reftable_record *b);\n-int reftable_record_equal(struct reftable_record *a, struct reftable_record *b, int hash_size);\n+int reftable_record_equal(struct reftable_record *a, struct reftable_record *b, uint32_t hash_size);\n int reftable_record_key(struct reftable_record *rec, struct reftable_buf *dest);\n int reftable_record_copy_from(struct reftable_record *rec,\n-\t\t\t struct reftable_record *src, int hash_size);\n+\t\t\t struct reftable_record *src, uint32_t hash_size);\n uint8_t reftable_record_val_type(struct reftable_record *rec);\n int reftable_record_encode(struct reftable_record *rec, struct string_view dest,\n-\t\t\t int hash_size);\n+\t\t\t uint32_t hash_size);\n int reftable_record_decode(struct reftable_record *rec, struct reftable_buf key,\n \t\t\t uint8_t extra, struct string_view src,\n-\t\t\t int hash_size, struct reftable_buf *scratch);\n+\t\t\t uint32_t hash_size, struct reftable_buf *scratch);\n int reftable_record_is_deletion(struct reftable_record *rec);\n \n static inline uint8_t reftable_record_type(struct reftable_record *rec)\ndiff --git a/reftable/reftable-record.h b/reftable/reftable-record.h\nindex ddd48eb579..931e594744 100644\n--- a/reftable/reftable-record.h\n+++ b/reftable/reftable-record.h\n@@ -65,7 +65,7 @@ void reftable_ref_record_release(struct reftable_ref_record *ref);\n \n /* returns whether two reftable_ref_records are the same. Useful for testing. */\n int reftable_ref_record_equal(const struct reftable_ref_record *a,\n-\t\t\t const struct reftable_ref_record *b, int hash_size);\n+\t\t\t const struct reftable_ref_record *b, uint32_t hash_size);\n \n /* reftable_log_record holds a reflog entry */\n struct reftable_log_record {\n@@ -105,6 +105,6 @@ void reftable_log_record_release(struct reftable_log_record *log);\n \n /* returns whether two records are equal. Useful for testing. */\n int reftable_log_record_equal(const struct reftable_log_record *a,\n-\t\t\t const struct reftable_log_record *b, int hash_size);\n+\t\t\t const struct reftable_log_record *b, uint32_t hash_size);\n \n #endif\ndiff --git a/t/unit-tests/t-reftable-record.c b/t/unit-tests/t-reftable-record.c\nindex 6d912b9c8f..d49d2a2729 100644\n--- a/t/unit-tests/t-reftable-record.c\n+++ b/t/unit-tests/t-reftable-record.c\n@@ -76,7 +76,7 @@ static void t_varint_overflow(void)\n \n static void set_hash(uint8_t *h, int j)\n {\n-\tfor (int i = 0; i < hash_size(REFTABLE_HASH_SHA1); i++)\n+\tfor (size_t i = 0; i < hash_size(REFTABLE_HASH_SHA1); i++)\n \t\th[i] = (j >> i) & 0xff;\n }\n \n\n-- \n2.48.0.257.gd3603152ad.dirty\n\n"},{"id":"510665","messageId":"20250116-b4-pks-reftable-sign-compare-v1-6-bd30e2ee96e7@pks.im","threadId":"62815","inReplyTo":"20250116-b4-pks-reftable-sign-compare-v1-0-bd30e2ee96e7@pks.im","subject":"[PATCH 06/10] reftable/block: adapt header and footer size to return a `size_t`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-16T10:08:38Z","receivedAt":"2025-01-16T10:08:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The functions `header_size()` and `footer_size()` return a positive\ninteger representing the size of the header and footer, respectively,\ndependent on the version of the reftable format. Similar to the\npreceding commit, these functions return a signed integer though, which\nis nonsensical given that there is no way for these functions to return\nnegative.\n\nAdapt the functions to return a `size_t` instead to fix a couple of sign\ncomparison warnings.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/block.c | 4 ++--\n reftable/block.h | 4 ++--\n t/unit-tests/t-reftable-readwrite.c | 2 +-\n 3 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/reftable/block.c b/reftable/block.c\nindex 2380aabb2f..1275085257 100644\n--- a/reftable/block.c\n+++ b/reftable/block.c\n@@ -15,7 +15,7 @@ license that can be found in the LICENSE file or at\n #include \"system.h\"\n #include \n \n-int header_size(int version)\n+size_t header_size(int version)\n {\n \tswitch (version) {\n \tcase 1:\n@@ -26,7 +26,7 @@ int header_size(int version)\n \tabort();\n }\n \n-int footer_size(int version)\n+size_t footer_size(int version)\n {\n \tswitch (version) {\n \tcase 1:\ndiff --git a/reftable/block.h b/reftable/block.h\nindex 5f67ed74c5..bef2b8a4c5 100644\n--- a/reftable/block.h\n+++ b/reftable/block.h\n@@ -137,10 +137,10 @@ void block_iter_reset(struct block_iter *it);\n void block_iter_close(struct block_iter *it);\n \n /* size of file header, depending on format version */\n-int header_size(int version);\n+size_t header_size(int version);\n \n /* size of file footer, depending on format version */\n-int footer_size(int version);\n+size_t footer_size(int version);\n \n /* returns a block to its source. */\n void reftable_block_done(struct reftable_block *ret);\ndiff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c\nindex 6b75a419b9..2e553154ea 100644\n--- a/t/unit-tests/t-reftable-readwrite.c\n+++ b/t/unit-tests/t-reftable-readwrite.c\n@@ -643,7 +643,7 @@ static void t_write_empty_table(void)\n \tcheck_int(err, ==, REFTABLE_EMPTY_TABLE_ERROR);\n \treftable_writer_free(w);\n \n-\tcheck_int(buf.len, ==, header_size(1) + footer_size(1));\n+\tcheck_uint(buf.len, ==, header_size(1) + footer_size(1));\n \n \tblock_source_from_buf(&source, &buf);\n \n\n-- \n2.48.0.257.gd3603152ad.dirty\n\n"},{"id":"510666","messageId":"20250116-b4-pks-reftable-sign-compare-v1-7-bd30e2ee96e7@pks.im","threadId":"62815","inReplyTo":"20250116-b4-pks-reftable-sign-compare-v1-0-bd30e2ee96e7@pks.im","subject":"[PATCH 07/10] reftable/block: adjust type of the restart length","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-16T10:08:39Z","receivedAt":"2025-01-16T10:08:48Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The restart length is tracked as a positive integer even though it\ncannot ever be negative. Furthermore, it is effectively capped via the\nMAX_RESTARTS variable.\n\nAdjust the type of the variable to be `uint32_t`. While this type is\nexcessive given that MAX_RESTARTS fits into an `uint16_t`, other places\nalready use 32 bit integers for restarts, so this type is being more\nconsistent.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/block.c | 12 +++++-------\n reftable/reftable-writer.h | 2 +-\n 2 files changed, 6 insertions(+), 8 deletions(-)\n\ndiff --git a/reftable/block.c b/reftable/block.c\nindex 1275085257..8ac865ce78 100644\n--- a/reftable/block.c\n+++ b/reftable/block.c\n@@ -40,16 +40,15 @@ size_t footer_size(int version)\n static int block_writer_register_restart(struct block_writer *w, int n,\n \t\t\t\t\t int is_restart, struct reftable_buf *key)\n {\n-\tint rlen, err;\n+\tuint32_t rlen;\n+\tint err;\n \n \trlen = w->restart_len;\n-\tif (rlen >= MAX_RESTARTS) {\n+\tif (rlen >= MAX_RESTARTS)\n \t\tis_restart = 0;\n-\t}\n \n-\tif (is_restart) {\n+\tif (is_restart)\n \t\trlen++;\n-\t}\n \tif (2 + 3 * rlen + n > w->block_size - w->next)\n \t\treturn -1;\n \tif (is_restart) {\n@@ -148,8 +147,7 @@ int block_writer_add(struct block_writer *w, struct reftable_record *rec)\n \n int block_writer_finish(struct block_writer *w)\n {\n-\tint i;\n-\tfor (i = 0; i < w->restart_len; i++) {\n+\tfor (uint32_t i = 0; i < w->restart_len; i++) {\n \t\tput_be24(w->block + w->next, w->restarts[i]);\n \t\tw->next += 3;\n \t}\ndiff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h\nindex 5f9afa620b..bfef3b1721 100644\n--- a/reftable/reftable-writer.h\n+++ b/reftable/reftable-writer.h\n@@ -84,7 +84,7 @@ struct reftable_block_stats {\n \t/* total number of entries written */\n \tint entries;\n \t/* total number of key restarts */\n-\tint restarts;\n+\tuint32_t restarts;\n \t/* total number of blocks */\n \tint blocks;\n \t/* total number of index blocks */\n\n-- \n2.48.0.257.gd3603152ad.dirty\n\n"},{"id":"510667","messageId":"20250116-b4-pks-reftable-sign-compare-v1-8-bd30e2ee96e7@pks.im","threadId":"62815","inReplyTo":"20250116-b4-pks-reftable-sign-compare-v1-0-bd30e2ee96e7@pks.im","subject":"[PATCH 08/10] reftable/blocksource: adjust type of the block length","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-16T10:08:40Z","receivedAt":"2025-01-16T10:08:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The block length is used to track the number of bytes available in a\nspecific block. As such, it is never set to a negative value, but is\nstill represented by a signed integer.\n\nAdjust the type of the variable to be `size_t`.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/reftable-blocksource.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/reftable/reftable-blocksource.h b/reftable/reftable-blocksource.h\nindex 5aa3990a57..f06ad52e0a 100644\n--- a/reftable/reftable-blocksource.h\n+++ b/reftable/reftable-blocksource.h\n@@ -22,7 +22,7 @@ struct reftable_block_source {\n * so it can return itself into the pool. */\n struct reftable_block {\n \tuint8_t *data;\n-\tint len;\n+\tsize_t len;\n \tstruct reftable_block_source source;\n };\n \n\n-- \n2.48.0.257.gd3603152ad.dirty\n\n"},{"id":"510668","messageId":"20250116-b4-pks-reftable-sign-compare-v1-9-bd30e2ee96e7@pks.im","threadId":"62815","inReplyTo":"20250116-b4-pks-reftable-sign-compare-v1-0-bd30e2ee96e7@pks.im","subject":"[PATCH 09/10] reftable/blocksource: adjust `read_block()` to return `ssize_t`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-16T10:08:41Z","receivedAt":"2025-01-16T10:08:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `block_source_read_block()` function and its implementations return\nan integer as a result that reflects either the number of bytes read, or\nan error. As such its return type, a signed integer, isn't wrong, but it\ndoesn't give the reader a good hint what it actually returns.\n\nRefactor the function to return an `ssize_t` instead, which is typical\nfor functions similar to read(3p) and should thus give readers a better\nsignal what they can expect as a result.\n\nAdjust callers to better handle the returned value to avoid warnings\nwith -Wsign-compare. One of these callers is `reader_get_block()`, whose\nreturn value is only ever used by its callers to figure out whether or\nnot the read was successful. So instead of bubbling up the `ssize_t`\nthere, too, we adapt it to only indicate success or errors.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/blocksource.c | 8 ++++----\n reftable/reader.c | 30 +++++++++++++++++-------------\n reftable/reader.h | 6 +++---\n reftable/reftable-blocksource.h | 11 +++++++----\n 4 files changed, 31 insertions(+), 24 deletions(-)\n\ndiff --git a/reftable/blocksource.c b/reftable/blocksource.c\nindex 52e0915a67..bba4a45b98 100644\n--- a/reftable/blocksource.c\n+++ b/reftable/blocksource.c\n@@ -24,8 +24,8 @@ static void reftable_buf_close(void *b UNUSED)\n {\n }\n \n-static int reftable_buf_read_block(void *v, struct reftable_block *dest,\n-\t\t\t\t uint64_t off, uint32_t size)\n+static ssize_t reftable_buf_read_block(void *v, struct reftable_block *dest,\n+\t\t\t\t uint64_t off, uint32_t size)\n {\n \tstruct reftable_buf *b = v;\n \tassert(off + size <= b->len);\n@@ -78,8 +78,8 @@ static void file_close(void *v)\n \treftable_free(b);\n }\n \n-static int file_read_block(void *v, struct reftable_block *dest, uint64_t off,\n-\t\t\t uint32_t size)\n+static ssize_t file_read_block(void *v, struct reftable_block *dest, uint64_t off,\n+\t\t\t uint32_t size)\n {\n \tstruct file_block_source *b = v;\n \tassert(off + size <= b->size);\ndiff --git a/reftable/reader.c b/reftable/reader.c\nindex 9df8a5ecb1..3f2e4b2800 100644\n--- a/reftable/reader.c\n+++ b/reftable/reader.c\n@@ -20,11 +20,11 @@ uint64_t block_source_size(struct reftable_block_source *source)\n \treturn source->ops->size(source->arg);\n }\n \n-int block_source_read_block(struct reftable_block_source *source,\n-\t\t\t struct reftable_block *dest, uint64_t off,\n-\t\t\t uint32_t size)\n+ssize_t block_source_read_block(struct reftable_block_source *source,\n+\t\t\t\tstruct reftable_block *dest, uint64_t off,\n+\t\t\t\tuint32_t size)\n {\n-\tint result = source->ops->read_block(source->arg, dest, off, size);\n+\tssize_t result = source->ops->read_block(source->arg, dest, off, size);\n \tdest->source = *source;\n \treturn result;\n }\n@@ -57,14 +57,17 @@ static int reader_get_block(struct reftable_reader *r,\n \t\t\t struct reftable_block *dest, uint64_t off,\n \t\t\t uint32_t sz)\n {\n+\tssize_t bytes_read;\n \tif (off >= r->size)\n \t\treturn 0;\n-\n-\tif (off + sz > r->size) {\n+\tif (off + sz > r->size)\n \t\tsz = r->size - off;\n-\t}\n \n-\treturn block_source_read_block(&r->source, dest, off, sz);\n+\tbytes_read = block_source_read_block(&r->source, dest, off, sz);\n+\tif (bytes_read < 0)\n+\t\treturn (int)bytes_read;\n+\n+\treturn 0;\n }\n \n enum reftable_hash reftable_reader_hash_id(struct reftable_reader *r)\n@@ -601,6 +604,7 @@ int reftable_reader_new(struct reftable_reader **out,\n \tstruct reftable_reader *r;\n \tuint64_t file_size = block_source_size(source);\n \tuint32_t read_size;\n+\tssize_t bytes_read;\n \tint err;\n \n \tREFTABLE_CALLOC_ARRAY(r, 1);\n@@ -619,8 +623,8 @@ int reftable_reader_new(struct reftable_reader **out,\n \t\tgoto done;\n \t}\n \n-\terr = block_source_read_block(source, &header, 0, read_size);\n-\tif (err != read_size) {\n+\tbytes_read = block_source_read_block(source, &header, 0, read_size);\n+\tif (bytes_read < 0 || (size_t)bytes_read != read_size) {\n \t\terr = REFTABLE_IO_ERROR;\n \t\tgoto done;\n \t}\n@@ -645,9 +649,9 @@ int reftable_reader_new(struct reftable_reader **out,\n \tr->hash_id = 0;\n \tr->refcount = 1;\n \n-\terr = block_source_read_block(source, &footer, r->size,\n-\t\t\t\t footer_size(r->version));\n-\tif (err != footer_size(r->version)) {\n+\tbytes_read = block_source_read_block(source, &footer, r->size,\n+\t\t\t\t\t footer_size(r->version));\n+\tif (bytes_read < 0 || (size_t)bytes_read != footer_size(r->version)) {\n \t\terr = REFTABLE_IO_ERROR;\n \t\tgoto done;\n \t}\ndiff --git a/reftable/reader.h b/reftable/reader.h\nindex d2b48a4849..bb72108a6f 100644\n--- a/reftable/reader.h\n+++ b/reftable/reader.h\n@@ -16,9 +16,9 @@ license that can be found in the LICENSE file or at\n \n uint64_t block_source_size(struct reftable_block_source *source);\n \n-int block_source_read_block(struct reftable_block_source *source,\n-\t\t\t struct reftable_block *dest, uint64_t off,\n-\t\t\t uint32_t size);\n+ssize_t block_source_read_block(struct reftable_block_source *source,\n+\t\t\t\tstruct reftable_block *dest, uint64_t off,\n+\t\t\t\tuint32_t size);\n void block_source_close(struct reftable_block_source *source);\n \n /* metadata for a block type */\ndiff --git a/reftable/reftable-blocksource.h b/reftable/reftable-blocksource.h\nindex f06ad52e0a..6b326aa5ea 100644\n--- a/reftable/reftable-blocksource.h\n+++ b/reftable/reftable-blocksource.h\n@@ -31,10 +31,13 @@ struct reftable_block_source_vtable {\n \t/* returns the size of a block source */\n \tuint64_t (*size)(void *source);\n \n-\t/* reads a segment from the block source. It is an error to read\n-\t beyond the end of the block */\n-\tint (*read_block)(void *source, struct reftable_block *dest,\n-\t\t\t uint64_t off, uint32_t size);\n+\t/*\n+\t * Reads a segment from the block source. It is an error to read beyond\n+\t * the end of the block.\n+\t */\n+\tssize_t (*read_block)(void *source, struct reftable_block *dest,\n+\t\t\t uint64_t off, uint32_t size);\n+\n \t/* mark the block as read; may return the data back to malloc */\n \tvoid (*return_block)(void *source, struct reftable_block *blockp);\n \n\n-- \n2.48.0.257.gd3603152ad.dirty\n\n"},{"id":"510669","messageId":"20250116-b4-pks-reftable-sign-compare-v1-10-bd30e2ee96e7@pks.im","threadId":"62815","inReplyTo":"20250116-b4-pks-reftable-sign-compare-v1-0-bd30e2ee96e7@pks.im","subject":"[PATCH 10/10] reftable: address trivial -Wsign-compare warnings","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-16T10:08:42Z","receivedAt":"2025-01-16T10:08:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Address the last couple of trivial -Wsign-compare warnings in the\nreftable library and remove the DISABLE_SIGN_COMPARE_WARNINGS macro that\nwe have in \"reftable/system.h\".\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/record.c | 7 ++-----\n reftable/stack.c | 12 +++++-------\n reftable/system.h | 2 --\n reftable/writer.c | 2 +-\n 4 files changed, 8 insertions(+), 15 deletions(-)\n\ndiff --git a/reftable/record.c b/reftable/record.c\nindex 0ce294078b..cff7564087 100644\n--- a/reftable/record.c\n+++ b/reftable/record.c\n@@ -117,7 +117,7 @@ static int decode_string(struct reftable_buf *dest, struct string_view in)\n static int encode_string(const char *str, struct string_view s)\n {\n \tstruct string_view start = s;\n-\tint l = strlen(str);\n+\tsize_t l = strlen(str);\n \tint n = put_var_int(&s, l);\n \tif (n < 0)\n \t\treturn -1;\n@@ -556,7 +556,6 @@ static int reftable_obj_record_decode(void *rec, struct reftable_buf key,\n \tuint64_t count = val_type;\n \tint n = 0;\n \tuint64_t last;\n-\tint j;\n \n \treftable_obj_record_release(r);\n \n@@ -591,8 +590,7 @@ static int reftable_obj_record_decode(void *rec, struct reftable_buf key,\n \tstring_view_consume(&in, n);\n \n \tlast = r->offsets[0];\n-\tj = 1;\n-\twhile (j < count) {\n+\tfor (uint64_t j = 1; j < count; j++) {\n \t\tuint64_t delta = 0;\n \t\tint n = get_var_int(&delta, &in);\n \t\tif (n < 0) {\n@@ -601,7 +599,6 @@ static int reftable_obj_record_decode(void *rec, struct reftable_buf key,\n \t\tstring_view_consume(&in, n);\n \n \t\tlast = r->offsets[j] = (delta + last);\n-\t\tj++;\n \t}\n \treturn start.len - in.len;\n }\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 531660a49f..5c0d6273a7 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -220,9 +220,9 @@ void reftable_stack_destroy(struct reftable_stack *st)\n \t}\n \n \tif (st->readers) {\n-\t\tint i = 0;\n \t\tstruct reftable_buf filename = REFTABLE_BUF_INIT;\n-\t\tfor (i = 0; i < st->readers_len; i++) {\n+\n+\t\tfor (size_t i = 0; i < st->readers_len; i++) {\n \t\t\tconst char *name = reader_name(st->readers[i]);\n \t\t\tint try_unlinking = 1;\n \n@@ -238,6 +238,7 @@ void reftable_stack_destroy(struct reftable_stack *st)\n \t\t\t\tunlink(filename.buf);\n \t\t\t}\n \t\t}\n+\n \t\treftable_buf_release(&filename);\n \t\tst->readers_len = 0;\n \t\tREFTABLE_FREE_AND_NULL(st->readers);\n@@ -568,7 +569,6 @@ static int stack_uptodate(struct reftable_stack *st)\n {\n \tchar **names = NULL;\n \tint err;\n-\tint i = 0;\n \n \t/*\n \t * When we have cached stat information available then we use it to\n@@ -608,7 +608,7 @@ static int stack_uptodate(struct reftable_stack *st)\n \tif (err < 0)\n \t\treturn err;\n \n-\tfor (i = 0; i < st->readers_len; i++) {\n+\tfor (size_t i = 0; i < st->readers_len; i++) {\n \t\tif (!names[i]) {\n \t\t\terr = 1;\n \t\t\tgoto done;\n@@ -1767,14 +1767,12 @@ static int reftable_stack_clean_locked(struct reftable_stack *st)\n \t}\n \n \twhile ((d = readdir(dir))) {\n-\t\tint i = 0;\n \t\tint found = 0;\n \t\tif (!is_table_name(d->d_name))\n \t\t\tcontinue;\n \n-\t\tfor (i = 0; !found && i < st->readers_len; i++) {\n+\t\tfor (size_t i = 0; !found && i < st->readers_len; i++)\n \t\t\tfound = !strcmp(reader_name(st->readers[i]), d->d_name);\n-\t\t}\n \t\tif (found)\n \t\t\tcontinue;\n \ndiff --git a/reftable/system.h b/reftable/system.h\nindex 5274eca1d0..7d5f803eeb 100644\n--- a/reftable/system.h\n+++ b/reftable/system.h\n@@ -11,8 +11,6 @@ license that can be found in the LICENSE file or at\n \n /* This header glues the reftable library to the rest of Git */\n \n-#define DISABLE_SIGN_COMPARE_WARNINGS\n-\n #include \"git-compat-util.h\"\n \n /*\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex 4e6ca2e368..91d6629486 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -577,7 +577,7 @@ static int writer_finish_section(struct reftable_writer *w)\n \n struct common_prefix_arg {\n \tstruct reftable_buf *last;\n-\tint max;\n+\tsize_t max;\n };\n \n static void update_common(void *void_arg, void *key)\n\n-- \n2.48.0.257.gd3603152ad.dirty\n\n"},{"id":"510742","messageId":"xmqqldvapfvy.fsf@gitster.g","threadId":"62815","inReplyTo":"20250116-b4-pks-reftable-sign-compare-v1-10-bd30e2ee96e7@pks.im","subject":"Re: [PATCH 10/10] reftable: address trivial -Wsign-compare warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-16T22:12:33Z","receivedAt":"2025-01-16T22:12:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Address the last couple of trivial -Wsign-compare warnings in the\n> reftable library and remove the DISABLE_SIGN_COMPARE_WARNINGS macro that\n> we have in \"reftable/system.h\".\n\nHmph.\n\n> -\tint l = strlen(str);\n> +\tsize_t l = strlen(str);\n\nObviously correct.\n\n> -\tj = 1;\n> -\twhile (j < count) {\n> +\tfor (uint64_t j = 1; j < count; j++) {\n\nOK. As count is u64, it makes sense to match.\n\n> diff --git a/reftable/stack.c b/reftable/stack.c\n> index 531660a49f..5c0d6273a7 100644\n> --- a/reftable/stack.c\n> +++ b/reftable/stack.c\n> @@ -220,9 +220,9 @@ void reftable_stack_destroy(struct reftable_stack *st)\n> \t}\n> \n> \tif (st->readers) {\n> -\t\tint i = 0;\n> \t\tstruct reftable_buf filename = REFTABLE_BUF_INIT;\n> -\t\tfor (i = 0; i < st->readers_len; i++) {\n> +\n> +\t\tfor (size_t i = 0; i < st->readers_len; i++) {\n\nLikewise, readers_len is size_t so counters can match it.\n\n> -\tfor (i = 0; i < st->readers_len; i++) {\n> +\tfor (size_t i = 0; i < st->readers_len; i++) {\n\nDitto.\n\n> -\t\tfor (i = 0; !found && i < st->readers_len; i++) {\n> +\t\tfor (size_t i = 0; !found && i < st->readers_len; i++)\n\nDitto.\n\n> diff --git a/reftable/writer.c b/reftable/writer.c\n> index 4e6ca2e368..91d6629486 100644\n> --- a/reftable/writer.c\n> +++ b/reftable/writer.c\n> @@ -577,7 +577,7 @@ static int writer_finish_section(struct reftable_writer *w)\n> \n> struct common_prefix_arg {\n> \tstruct reftable_buf *last;\n> -\tint max;\n> +\tsize_t max;\n> };\n\nThis is dubious. write.c:update_common() uses this to keep track of\nthe maximum value returned by common_prefix_size(), which returns\nan int. writer.c:writer_dump_object_index() assigns the value\ncomparable to this member to reftable_stats.object_id_len that is of\ntype int.\n\nI may be more sympathetic if we were making common_prefix_size()\nreturn size_t instead of \"int\" and dealing with all the fallouts\nfrom such a change, but this smells more like somebody _else_ is\nusing size_t on something that is not an allocation size, and such\nmixed use is cascading down to contaminate this member, which would\nbe perfectly fine with platform native \"int\".\n\nAh, OK, an earlier patch does change these other things to size_t,\nso this must change to size_t to be consistent. Shouldn't it be\ndone in the same patch, then, though?\n"},{"id":"510755","messageId":"Z4n0U62eBD4UiC8n@pks.im","threadId":"62815","inReplyTo":"xmqqldvapfvy.fsf@gitster.g","subject":"Re: [PATCH 10/10] reftable: address trivial -Wsign-compare warnings","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-17T06:10:27Z","receivedAt":"2025-01-17T06:10:30Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Jan 16, 2025 at 02:12:33PM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> > diff --git a/reftable/writer.c b/reftable/writer.c\n> > index 4e6ca2e368..91d6629486 100644\n> > --- a/reftable/writer.c\n> > +++ b/reftable/writer.c\n> > @@ -577,7 +577,7 @@ static int writer_finish_section(struct reftable_writer *w)\n> > \n> > struct common_prefix_arg {\n> > \tstruct reftable_buf *last;\n> > -\tint max;\n> > +\tsize_t max;\n> > };\n> \n> This is dubious. write.c:update_common() uses this to keep track of\n> the maximum value returned by common_prefix_size(), which returns\n> an int. writer.c:writer_dump_object_index() assigns the value\n> comparable to this member to reftable_stats.object_id_len that is of\n> type int.\n> \n> I may be more sympathetic if we were making common_prefix_size()\n> return size_t instead of \"int\" and dealing with all the fallouts\n> from such a change, but this smells more like somebody _else_ is\n> using size_t on something that is not an allocation size, and such\n> mixed use is cascading down to contaminate this member, which would\n> be perfectly fine with platform native \"int\".\n> \n> Ah, OK, an earlier patch does change these other things to size_t,\n> so this must change to size_t to be consistent. Shouldn't it be\n> done in the same patch, then, though?\n\nGood point indeed. I'll move the change around, thanks!\n\nPatrick\n"},{"id":"510935","messageId":"CAOLa=ZQxp=tmmBwAV2OR9ODLGf_VHLxG_50-YwN7-s7+c6pmNQ@mail.gmail.com","threadId":"62815","inReplyTo":"20250116-b4-pks-reftable-sign-compare-v1-3-bd30e2ee96e7@pks.im","subject":"Re: [PATCH 03/10] reftable/record: handle overflows when decoding varints","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-01-20T09:47:47Z","receivedAt":"2025-01-20T09:47:49Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n[snip]\n\n> diff --git a/reftable/record.c b/reftable/record.c\n> index 04429d23fe..4e6541c307 100644\n> --- a/reftable/record.c\n> +++ b/reftable/record.c\n> @@ -21,47 +21,40 @@ static void *reftable_record_data(struct reftable_record *rec);\n>\n> int get_var_int(uint64_t *dest, struct string_view *in)\n> {\n> -\tint ptr = 0;\n> +\tconst unsigned char *buf = in->buf;\n> +\tunsigned char c;\n> \tuint64_t val;\n>\n> -\tif (in->len == 0)\n> +\tif (!in->len)\n> \t\treturn -1;\n> -\tval = in->buf[ptr] & 0x7f;\n> -\n> -\twhile (in->buf[ptr] & 0x80) {\n> -\t\tptr++;\n> -\t\tif (ptr > in->len) {\n> +\tc = *buf++;\n> +\tval = c & 0x7f;\n> +\n> +\twhile (c & 0x80) {\n> +\t\tval += 1;\n\nI was at first confused, I understand that we add 1 to check if there is\nan overflow before adding the next section. But this actually modifies\nthe value itself, but looking below at `put_var_int()`, we did value--\nbefore storing each continuation byte. So during decoding.\n\nNit: it would be nice to explain that part a bit here with comments.\n\n> +\t\tif (!val || (val & (uint64_t)(~0ULL << (64 - 7))))\n> +\t\t\treturn -1; /* overflow */\n> +\t\tif (buf >= in->buf + in->len)\n> \t\t\treturn -1;\n> -\t\t}\n> -\t\tval = (val + 1) << 7 | (uint64_t)(in->buf[ptr] & 0x7f);\n> +\t\tc = *buf++;\n> +\t\tval = (val << 7) | (c & 0x7f);\n> \t}\n>\n> \t*dest = val;\n> -\treturn ptr + 1;\n> +\treturn buf - in->buf;\n> }\n>\n> -int put_var_int(struct string_view *dest, uint64_t val)\n> +int put_var_int(struct string_view *dest, uint64_t value)\n> {\n> -\tuint8_t buf[10] = { 0 };\n> -\tint i = 9;\n> -\tint n = 0;\n> -\tbuf[i] = (uint8_t)(val & 0x7f);\n> -\ti--;\n> -\twhile (1) {\n> -\t\tval >>= 7;\n> -\t\tif (!val) {\n> -\t\t\tbreak;\n> -\t\t}\n> -\t\tval--;\n> -\t\tbuf[i] = 0x80 | (uint8_t)(val & 0x7f);\n> -\t\ti--;\n> -\t}\n> -\n> -\tn = sizeof(buf) - i - 1;\n> -\tif (dest->len < n)\n> +\tunsigned char varint[10];\n> +\tunsigned pos = sizeof(varint) - 1;\n> +\tvarint[pos] = value & 127;\n\nNit: While the `get_var_int()` uses hexes, here we use ints. Would be\nnicer to use `0x7f` and so on and be consistent.\n\n> +\twhile (value >>= 7)\n> +\t\tvarint[--pos] = 128 | (--value & 127);\n> +\tif (dest->len < sizeof(varint) - pos)\n> \t\treturn -1;\n> -\tmemcpy(dest->buf, &buf[i + 1], n);\n> -\treturn n;\n> +\tmemcpy(dest->buf, varint + pos, sizeof(varint) - pos);\n> +\treturn sizeof(varint) - pos;\n> }\n>\n> int reftable_is_block_type(uint8_t typ)\n> diff --git a/reftable/record.h b/reftable/record.h\n> index a24cb23bd4..721d6c949a 100644\n> --- a/reftable/record.h\n> +++ b/reftable/record.h\n> @@ -34,6 +34,10 @@ static inline void string_view_consume(struct string_view *s, int n)\n>\n> /* utilities for de/encoding varints */\n>\n\nWe should remove this, no?\n\n> +/*\n> + * Decode and encode a varint. Returns the number of bytes read/written, or a\n> + * negative value in case encoding/decoding the varint has failed.\n> + */\n> int get_var_int(uint64_t *dest, struct string_view *in);\n> int put_var_int(struct string_view *dest, uint64_t val);\n>\n> diff --git a/t/unit-tests/t-reftable-record.c b/t/unit-tests/t-reftable-record.c\n> index 42bc64cec8..6d912b9c8f 100644\n> --- a/t/unit-tests/t-reftable-record.c\n> +++ b/t/unit-tests/t-reftable-record.c\n> @@ -58,6 +58,22 @@ static void t_varint_roundtrip(void)\n> \t}\n> }\n\n[snip]\n"},{"id":"510936","messageId":"CAOLa=ZQXF8ewDVmusbXPSCPPp9qqj2JRTvZeiiDZRckG5BpV6g@mail.gmail.com","threadId":"62815","inReplyTo":"20250116-b4-pks-reftable-sign-compare-v1-0-bd30e2ee96e7@pks.im","subject":"Re: [PATCH 00/10] reftable: fix -Wsign-compare warnings","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-01-20T10:07:23Z","receivedAt":"2025-01-20T10:07:25Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> Hi,\n>\n> during the last steps of converting the reftable codebase to become a\n> standalone library I noticed that the new -Wsign-compare warnings\n> created a bit of a problem due to the `DISABLE_SIGN_COMPARE_WARNINGS`\n> macro that we started using. As a consequence I wasn't able to easily\n> drop \"git-compat-util.h\" anymore. This patch series is thus addresses\n> the issue by fixing all sign comparison warnings in the reftable\n> library.\n>\n> Thanks!\n>\n\nMost of the patches were straightforward and look good. I left only nits\non one commit, which doesn't warrant a re-roll.\n\nThanks\n\n[snip]\n"},{"id":"510950","messageId":"Z45nMqLUHwMGEy11@pks.im","threadId":"62815","inReplyTo":"CAOLa=ZQxp=tmmBwAV2OR9ODLGf_VHLxG_50-YwN7-s7+c6pmNQ@mail.gmail.com","subject":"Re: [PATCH 03/10] reftable/record: handle overflows when decoding varints","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-20T15:09:48Z","receivedAt":"2025-01-20T15:09:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jan 20, 2025 at 04:47:47AM -0500, Karthik Nayak wrote:\n> Patrick Steinhardt writes:\n> > diff --git a/reftable/record.c b/reftable/record.c\n> > index 04429d23fe..4e6541c307 100644\n> > --- a/reftable/record.c\n> > +++ b/reftable/record.c\n> > @@ -21,47 +21,40 @@ static void *reftable_record_data(struct reftable_record *rec);\n> >\n> > int get_var_int(uint64_t *dest, struct string_view *in)\n> > {\n> > -\tint ptr = 0;\n> > +\tconst unsigned char *buf = in->buf;\n> > +\tunsigned char c;\n> > \tuint64_t val;\n> >\n> > -\tif (in->len == 0)\n> > +\tif (!in->len)\n> > \t\treturn -1;\n> > -\tval = in->buf[ptr] & 0x7f;\n> > -\n> > -\twhile (in->buf[ptr] & 0x80) {\n> > -\t\tptr++;\n> > -\t\tif (ptr > in->len) {\n> > +\tc = *buf++;\n> > +\tval = c & 0x7f;\n> > +\n> > +\twhile (c & 0x80) {\n> > +\t\tval += 1;\n> \n> I was at first confused, I understand that we add 1 to check if there is\n> an overflow before adding the next section. But this actually modifies\n> the value itself, but looking below at `put_var_int()`, we did value--\n> before storing each continuation byte. So during decoding.\n> \n> Nit: it would be nice to explain that part a bit here with comments.\n\nYeah, I had to think about it a bit myself. It's quite a clever\noptimization: when the 0x80 bit is set, we know that the remaining value\ncannot be 0. We thus don't have to represent that value, which is why we\ncan subtract 1 when encoding and re-add 1 when decoding. This allows us\nto save a byte in some edge cases.\n\n[snip]\n> > -int put_var_int(struct string_view *dest, uint64_t val)\n> > +int put_var_int(struct string_view *dest, uint64_t value)\n> > {\n> > -\tuint8_t buf[10] = { 0 };\n> > -\tint i = 9;\n> > -\tint n = 0;\n> > -\tbuf[i] = (uint8_t)(val & 0x7f);\n> > -\ti--;\n> > -\twhile (1) {\n> > -\t\tval >>= 7;\n> > -\t\tif (!val) {\n> > -\t\t\tbreak;\n> > -\t\t}\n> > -\t\tval--;\n> > -\t\tbuf[i] = 0x80 | (uint8_t)(val & 0x7f);\n> > -\t\ti--;\n> > -\t}\n> > -\n> > -\tn = sizeof(buf) - i - 1;\n> > -\tif (dest->len < n)\n> > +\tunsigned char varint[10];\n> > +\tunsigned pos = sizeof(varint) - 1;\n> > +\tvarint[pos] = value & 127;\n> \n> Nit: While the `get_var_int()` uses hexes, here we use ints. Would be\n> nicer to use `0x7f` and so on and be consistent.\n\nYup, makes sense.\n\n> > +\twhile (value >>= 7)\n> > +\t\tvarint[--pos] = 128 | (--value & 127);\n> > +\tif (dest->len < sizeof(varint) - pos)\n> > \t\treturn -1;\n> > -\tmemcpy(dest->buf, &buf[i + 1], n);\n> > -\treturn n;\n> > +\tmemcpy(dest->buf, varint + pos, sizeof(varint) - pos);\n> > +\treturn sizeof(varint) - pos;\n> > }\n> >\n> > int reftable_is_block_type(uint8_t typ)\n> > diff --git a/reftable/record.h b/reftable/record.h\n> > index a24cb23bd4..721d6c949a 100644\n> > --- a/reftable/record.h\n> > +++ b/reftable/record.h\n> > @@ -34,6 +34,10 @@ static inline void string_view_consume(struct string_view *s, int n)\n> >\n> > /* utilities for de/encoding varints */\n> >\n> \n> We should remove this, no?\n\nYup, good catch.\n\nPatrick\n"},{"id":"510951","messageId":"Z45nTojf3D6hDdMk@pks.im","threadId":"62815","inReplyTo":"CAOLa=ZQXF8ewDVmusbXPSCPPp9qqj2JRTvZeiiDZRckG5BpV6g@mail.gmail.com","subject":"Re: [PATCH 00/10] reftable: fix -Wsign-compare warnings","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-20T15:10:06Z","receivedAt":"2025-01-20T15:10:09Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jan 20, 2025 at 05:07:23AM -0500, Karthik Nayak wrote:\n> Patrick Steinhardt writes:\n> \n> > Hi,\n> >\n> > during the last steps of converting the reftable codebase to become a\n> > standalone library I noticed that the new -Wsign-compare warnings\n> > created a bit of a problem due to the `DISABLE_SIGN_COMPARE_WARNINGS`\n> > macro that we started using. As a consequence I wasn't able to easily\n> > drop \"git-compat-util.h\" anymore. This patch series is thus addresses\n> > the issue by fixing all sign comparison warnings in the reftable\n> > library.\n> >\n> > Thanks!\n> >\n> \n> Most of the patches were straightforward and look good. I left only nits\n> on one commit, which doesn't warrant a re-roll.\n\nThanks for your review! I'll send v2 in a bit.\n\nPatrick\n"},{"id":"510956","messageId":"20250120-b4-pks-reftable-sign-compare-v2-0-b4566d02e4a5@pks.im","threadId":"62815","inReplyTo":"20250116-b4-pks-reftable-sign-compare-v1-0-bd30e2ee96e7@pks.im","subject":"[PATCH v2 00/10] reftable: fix -Wsign-compare warnings","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-20T16:17:18Z","receivedAt":"2025-01-20T16:17:29Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nduring the last steps of converting the reftable codebase to become a\nstandalone library I noticed that the new -Wsign-compare warnings\ncreated a bit of a problem due to the `DISABLE_SIGN_COMPARE_WARNINGS`\nmacro that we started using. As a consequence I wasn't able to easily\ndrop \"git-compat-util.h\" anymore. This patch series is thus addresses\nthe issue by fixing all sign comparison warnings in the reftable\nlibrary.\n\nChanges in v2:\n - Document why we add/subtract 1 when encoding and decoding varints.\n - Constistently use hex representations when encoding varints.\n - Use `+` instead of `|` to add values in varint decoding -- it\n shouldn't make a difference, but is closer to what Git's own\n algorithm uses.\n - Link to v1: https://lore.kernel.org/r/20250116-b4-pks-reftable-sign-compare-v1-0-bd30e2ee96e7@pks.im\n\nThanks!\n\nPatrick\n\n---\nPatrick Steinhardt (10):\n meson: stop disabling -Wsign-compare\n reftable/record: drop unused `print` function pointer\n reftable/record: handle overflows when decoding varints\n reftable/basics: adjust `common_prefix_size()` to return `size_t`\n reftable/basics: adjust `hash_size()` to return `uint32_t`\n reftable/block: adapt header and footer size to return a `size_t`\n reftable/block: adjust type of the restart length\n reftable/blocksource: adjust type of the block length\n reftable/blocksource: adjust `read_block()` to return `ssize_t`\n reftable: address trivial -Wsign-compare warnings\n\n meson.build | 1 -\n reftable/basics.c | 10 ++-\n reftable/basics.h | 4 +-\n reftable/block.c | 20 +++---\n reftable/block.h | 14 ++--\n reftable/blocksource.c | 8 +--\n reftable/reader.c | 32 +++++----\n reftable/reader.h | 6 +-\n reftable/record.c | 125 +++++++++++++++++-------------------\n reftable/record.h | 25 ++++----\n reftable/reftable-blocksource.h | 13 ++--\n reftable/reftable-record.h | 4 +-\n reftable/reftable-writer.h | 2 +-\n reftable/stack.c | 12 ++--\n reftable/system.h | 2 -\n reftable/writer.c | 7 +-\n t/unit-tests/t-reftable-basics.c | 2 +-\n t/unit-tests/t-reftable-readwrite.c | 2 +-\n t/unit-tests/t-reftable-record.c | 19 +++++-\n 19 files changed, 157 insertions(+), 151 deletions(-)\n\nRange-diff versus v1:\n\n 1: 2fb873ee80 = 1: dcc0fa1906 meson: stop disabling -Wsign-compare\n 2: da4f0ce11e = 2: 059ee6afe1 reftable/record: drop unused `print` function pointer\n 3: 6f242d9af0 ! 3: 2a50096d14 reftable/record: handle overflows when decoding varints\n @@ Commit message\n implementation is basically copied from Git's own `decode_varint()`,\n which already knows to handle overflows. The only adjustment is that we\n also take into account the string view's length in order to not overrun\n - it.\n + it. The reftable documentation explicitly notes that those two encoding\n + schemas are supposed to be the same:\n +\n + Varint encoding\n + ^^^^^^^^^^^^^^^\n +\n + Varint encoding is identical to the ofs-delta encoding method used\n + within pack files.\n +\n + Decoder works as follows:\n +\n + ....\n + val = buf[ptr] & 0x7f\n + while (buf[ptr] & 0x80) {\n + ptr++\n + val = ((val + 1) << 7) | (buf[ptr] & 0x7f)\n + }\n + ....\n \n While at it, refactor `put_var_int()` in the same way by copying over\n the implementation of `encode_varint()`. While `put_var_int()` doesn't\n @@ reftable/record.c: static void *reftable_record_data(struct reftable_record *rec\n +\tval = c & 0x7f;\n +\n +\twhile (c & 0x80) {\n ++\t\t/*\n ++\t\t * We use a micro-optimization here: whenever we see that the\n ++\t\t * 0x80 bit is set, we know that the remainder of the value\n ++\t\t * cannot be 0. The zero-values thus doesn't need to be encoded\n ++\t\t * at all, which is why we subtract 1 when encoding and add 1\n ++\t\t * when decoding.\n ++\t\t *\n ++\t\t * This allows us to save a byte in some edge cases.\n ++\t\t */\n +\t\tval += 1;\n +\t\tif (!val || (val & (uint64_t)(~0ULL << (64 - 7))))\n +\t\t\treturn -1; /* overflow */\n @@ reftable/record.c: static void *reftable_record_data(struct reftable_record *rec\n -\t\t}\n -\t\tval = (val + 1) << 7 | (uint64_t)(in->buf[ptr] & 0x7f);\n +\t\tc = *buf++;\n -+\t\tval = (val << 7) | (c & 0x7f);\n ++\t\tval = (val << 7) + (c & 0x7f);\n \t}\n \n \t*dest = val;\n @@ reftable/record.c: static void *reftable_record_data(struct reftable_record *rec\n -\tif (dest->len < n)\n +\tunsigned char varint[10];\n +\tunsigned pos = sizeof(varint) - 1;\n -+\tvarint[pos] = value & 127;\n ++\tvarint[pos] = value & 0x7f;\n +\twhile (value >>= 7)\n -+\t\tvarint[--pos] = 128 | (--value & 127);\n ++\t\tvarint[--pos] = 0x80 | (--value & 0x7f);\n +\tif (dest->len < sizeof(varint) - pos)\n \t\treturn -1;\n -\tmemcpy(dest->buf, &buf[i + 1], n);\n @@ reftable/record.c: static void *reftable_record_data(struct reftable_record *rec\n \n ## reftable/record.h ##\n @@ reftable/record.h: static inline void string_view_consume(struct string_view *s, int n)\n + \ts->len -= n;\n + }\n \n - /* utilities for de/encoding varints */\n - \n +-/* utilities for de/encoding varints */\n +-\n +/*\n + * Decode and encode a varint. Returns the number of bytes read/written, or a\n + * negative value in case encoding/decoding the varint has failed.\n 4: e109babe15 ! 4: 227002d330 reftable/basics: adjust `common_prefix_size()` to return `size_t`\n @@ reftable/record.c: int reftable_encode_key(int *restart, struct string_view dest\n \tstring_view_consume(&dest, n);\n \n ## reftable/writer.c ##\n +@@ reftable/writer.c: static int writer_finish_section(struct reftable_writer *w)\n + \n + struct common_prefix_arg {\n + \tstruct reftable_buf *last;\n +-\tint max;\n ++\tsize_t max;\n + };\n + \n + static void update_common(void *void_arg, void *key)\n @@ reftable/writer.c: static void update_common(void *void_arg, void *key)\n \tstruct common_prefix_arg *arg = void_arg;\n \tstruct obj_index_tree_node *entry = key;\n 5: bf9a568639 = 5: 63e709b44f reftable/basics: adjust `hash_size()` to return `uint32_t`\n 6: 65b7137b90 = 6: 7f620bfeca reftable/block: adapt header and footer size to return a `size_t`\n 7: 88748fd8a1 = 7: e679e37e32 reftable/block: adjust type of the restart length\n 8: 7d8c0eda59 = 8: 43d20a41e0 reftable/blocksource: adjust type of the block length\n 9: 048be4c779 = 9: 48cccb5c00 reftable/blocksource: adjust `read_block()` to return `ssize_t`\n10: 31b97b00f4 ! 10: 83f35b88d4 reftable: address trivial -Wsign-compare warnings\n @@ reftable/system.h: license that can be found in the LICENSE file or at\n #include \"git-compat-util.h\"\n \n /*\n -\n - ## reftable/writer.c ##\n -@@ reftable/writer.c: static int writer_finish_section(struct reftable_writer *w)\n - \n - struct common_prefix_arg {\n - \tstruct reftable_buf *last;\n --\tint max;\n -+\tsize_t max;\n - };\n - \n - static void update_common(void *void_arg, void *key)\n\n---\nbase-commit: 757161efcca150a9a96b312d9e780a071e601a03\nchange-id: 20250116-b4-pks-reftable-sign-compare-8eaabae74c06\n\n"},{"id":"510957","messageId":"20250120-b4-pks-reftable-sign-compare-v2-1-b4566d02e4a5@pks.im","threadId":"62815","inReplyTo":"20250120-b4-pks-reftable-sign-compare-v2-0-b4566d02e4a5@pks.im","subject":"[PATCH v2 01/10] meson: stop disabling -Wsign-compare","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-20T16:17:19Z","receivedAt":"2025-01-20T16:17:29Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In 4f9264b0cd (config.mak.dev: drop `-Wno-sign-compare`, 2024-12-06) we\nhave started an effort to make our codebase compile with -Wsign-compare.\nBut while we removed the -Wno-sign-compare flag from \"config.mak.dev\",\nwe didn't adjust the Meson build instructions in the same way.\n\nFix this.\n\nSigned-off-by: Patrick Steinhardt \n---\n meson.build | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/meson.build b/meson.build\nindex 0064eb64f5..07744c73b1 100644\n--- a/meson.build\n+++ b/meson.build\n@@ -708,7 +708,6 @@ if get_option('warning_level') in ['2','3', 'everything'] and compiler.get_argum\n # These are disabled because we have these all over the place.\n '-Wno-empty-body',\n '-Wno-missing-field-initializers',\n- '-Wno-sign-compare',\n ]\n if compiler.has_argument(cflag)\n libgit_c_args += cflag\n\n-- \n2.48.0.257.gd3603152ad.dirty\n\n"},{"id":"510958","messageId":"20250120-b4-pks-reftable-sign-compare-v2-2-b4566d02e4a5@pks.im","threadId":"62815","inReplyTo":"20250120-b4-pks-reftable-sign-compare-v2-0-b4566d02e4a5@pks.im","subject":"[PATCH v2 02/10] reftable/record: drop unused `print` function pointer","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-20T16:17:20Z","receivedAt":"2025-01-20T16:17:30Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In 42c424d69d (t/helper: inline printing of reftable records,\n2024-08-22) we stopped using the `print` function of the reftable record\nvtable and instead moved its implementation into the single user of it.\nWe didn't remove the function itself from the vtable though. Drop it.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/record.h | 3 ---\n 1 file changed, 3 deletions(-)\n\ndiff --git a/reftable/record.h b/reftable/record.h\nindex 25aa908c85..a24cb23bd4 100644\n--- a/reftable/record.h\n+++ b/reftable/record.h\n@@ -73,9 +73,6 @@ struct reftable_record_vtable {\n \t * the same type.\n \t */\n \tint (*cmp)(const void *a, const void *b);\n-\n-\t/* Print on stdout, for debugging. */\n-\tvoid (*print)(const void *rec, int hash_size);\n };\n \n /* returns true for recognized block types. Block start with the block type. */\n\n-- \n2.48.0.257.gd3603152ad.dirty\n\n"},{"id":"510959","messageId":"20250120-b4-pks-reftable-sign-compare-v2-3-b4566d02e4a5@pks.im","threadId":"62815","inReplyTo":"20250120-b4-pks-reftable-sign-compare-v2-0-b4566d02e4a5@pks.im","subject":"[PATCH v2 03/10] reftable/record: handle overflows when decoding varints","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-20T16:17:21Z","receivedAt":"2025-01-20T16:17:30Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The logic to decode varints isn't able to detect integer overflows: as\nlong as the buffer still has more data available, and as long as the\ncurrent byte has its 0x80 bit set, we'll continue to add up these values\nto the result. This will eventually cause the `uint64_t` to overflow, at\nwhich point we'll return an invalid result.\n\nRefactor the function so that it is able to detect such overflows. The\nimplementation is basically copied from Git's own `decode_varint()`,\nwhich already knows to handle overflows. The only adjustment is that we\nalso take into account the string view's length in order to not overrun\nit. The reftable documentation explicitly notes that those two encoding\nschemas are supposed to be the same:\n\n Varint encoding\n ^^^^^^^^^^^^^^^\n\n Varint encoding is identical to the ofs-delta encoding method used\n within pack files.\n\n Decoder works as follows:\n\n ....\n val = buf[ptr] & 0x7f\n while (buf[ptr] & 0x80) {\n ptr++\n val = ((val + 1) << 7) | (buf[ptr] & 0x7f)\n }\n ....\n\nWhile at it, refactor `put_var_int()` in the same way by copying over\nthe implementation of `encode_varint()`. While `put_var_int()` doesn't\nhave an issue with overflows, it generates warnings with -Wsign-compare.\nThe implementation of `encode_varint()` doesn't, is battle-tested and at\nthe same time way simpler than what we currently have.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/record.c | 62 +++++++++++++++++++++-------------------\n reftable/record.h | 6 ++--\n t/unit-tests/t-reftable-record.c | 17 +++++++++++\n 3 files changed, 53 insertions(+), 32 deletions(-)\n\ndiff --git a/reftable/record.c b/reftable/record.c\nindex 04429d23fe..a55ce76aeb 100644\n--- a/reftable/record.c\n+++ b/reftable/record.c\n@@ -21,47 +21,49 @@ static void *reftable_record_data(struct reftable_record *rec);\n \n int get_var_int(uint64_t *dest, struct string_view *in)\n {\n-\tint ptr = 0;\n+\tconst unsigned char *buf = in->buf;\n+\tunsigned char c;\n \tuint64_t val;\n \n-\tif (in->len == 0)\n+\tif (!in->len)\n \t\treturn -1;\n-\tval = in->buf[ptr] & 0x7f;\n-\n-\twhile (in->buf[ptr] & 0x80) {\n-\t\tptr++;\n-\t\tif (ptr > in->len) {\n+\tc = *buf++;\n+\tval = c & 0x7f;\n+\n+\twhile (c & 0x80) {\n+\t\t/*\n+\t\t * We use a micro-optimization here: whenever we see that the\n+\t\t * 0x80 bit is set, we know that the remainder of the value\n+\t\t * cannot be 0. The zero-values thus doesn't need to be encoded\n+\t\t * at all, which is why we subtract 1 when encoding and add 1\n+\t\t * when decoding.\n+\t\t *\n+\t\t * This allows us to save a byte in some edge cases.\n+\t\t */\n+\t\tval += 1;\n+\t\tif (!val || (val & (uint64_t)(~0ULL << (64 - 7))))\n+\t\t\treturn -1; /* overflow */\n+\t\tif (buf >= in->buf + in->len)\n \t\t\treturn -1;\n-\t\t}\n-\t\tval = (val + 1) << 7 | (uint64_t)(in->buf[ptr] & 0x7f);\n+\t\tc = *buf++;\n+\t\tval = (val << 7) + (c & 0x7f);\n \t}\n \n \t*dest = val;\n-\treturn ptr + 1;\n+\treturn buf - in->buf;\n }\n \n-int put_var_int(struct string_view *dest, uint64_t val)\n+int put_var_int(struct string_view *dest, uint64_t value)\n {\n-\tuint8_t buf[10] = { 0 };\n-\tint i = 9;\n-\tint n = 0;\n-\tbuf[i] = (uint8_t)(val & 0x7f);\n-\ti--;\n-\twhile (1) {\n-\t\tval >>= 7;\n-\t\tif (!val) {\n-\t\t\tbreak;\n-\t\t}\n-\t\tval--;\n-\t\tbuf[i] = 0x80 | (uint8_t)(val & 0x7f);\n-\t\ti--;\n-\t}\n-\n-\tn = sizeof(buf) - i - 1;\n-\tif (dest->len < n)\n+\tunsigned char varint[10];\n+\tunsigned pos = sizeof(varint) - 1;\n+\tvarint[pos] = value & 0x7f;\n+\twhile (value >>= 7)\n+\t\tvarint[--pos] = 0x80 | (--value & 0x7f);\n+\tif (dest->len < sizeof(varint) - pos)\n \t\treturn -1;\n-\tmemcpy(dest->buf, &buf[i + 1], n);\n-\treturn n;\n+\tmemcpy(dest->buf, varint + pos, sizeof(varint) - pos);\n+\treturn sizeof(varint) - pos;\n }\n \n int reftable_is_block_type(uint8_t typ)\ndiff --git a/reftable/record.h b/reftable/record.h\nindex a24cb23bd4..0df950f401 100644\n--- a/reftable/record.h\n+++ b/reftable/record.h\n@@ -32,8 +32,10 @@ static inline void string_view_consume(struct string_view *s, int n)\n \ts->len -= n;\n }\n \n-/* utilities for de/encoding varints */\n-\n+/*\n+ * Decode and encode a varint. Returns the number of bytes read/written, or a\n+ * negative value in case encoding/decoding the varint has failed.\n+ */\n int get_var_int(uint64_t *dest, struct string_view *in);\n int put_var_int(struct string_view *dest, uint64_t val);\n \ndiff --git a/t/unit-tests/t-reftable-record.c b/t/unit-tests/t-reftable-record.c\nindex 42bc64cec8..6d912b9c8f 100644\n--- a/t/unit-tests/t-reftable-record.c\n+++ b/t/unit-tests/t-reftable-record.c\n@@ -58,6 +58,22 @@ static void t_varint_roundtrip(void)\n \t}\n }\n \n+static void t_varint_overflow(void)\n+{\n+\tunsigned char buf[] = {\n+\t\t0xFF, 0xFF, 0xFF, 0xFF,\n+\t\t0xFF, 0xFF, 0xFF, 0xFF,\n+\t\t0xFF, 0x00,\n+\t};\n+\tstruct string_view view = {\n+\t\t.buf = buf,\n+\t\t.len = sizeof(buf),\n+\t};\n+\tuint64_t value;\n+\tint err = get_var_int(&value, &view);\n+\tcheck_int(err, ==, -1);\n+}\n+\n static void set_hash(uint8_t *h, int j)\n {\n \tfor (int i = 0; i < hash_size(REFTABLE_HASH_SHA1); i++)\n@@ -544,6 +560,7 @@ int cmd_main(int argc UNUSED, const char *argv[] UNUSED)\n \tTEST(t_reftable_log_record_roundtrip(), \"record operations work on log record\");\n \tTEST(t_reftable_ref_record_roundtrip(), \"record operations work on ref record\");\n \tTEST(t_varint_roundtrip(), \"put_var_int and get_var_int work\");\n+\tTEST(t_varint_overflow(), \"get_var_int notices an integer overflow\");\n \tTEST(t_key_roundtrip(), \"reftable_encode_key and reftable_decode_key work\");\n \tTEST(t_reftable_obj_record_roundtrip(), \"record operations work on obj record\");\n \tTEST(t_reftable_index_record_roundtrip(), \"record operations work on index record\");\n\n-- \n2.48.0.257.gd3603152ad.dirty\n\n"},{"id":"510960","messageId":"20250120-b4-pks-reftable-sign-compare-v2-4-b4566d02e4a5@pks.im","threadId":"62815","inReplyTo":"20250120-b4-pks-reftable-sign-compare-v2-0-b4566d02e4a5@pks.im","subject":"[PATCH v2 04/10] reftable/basics: adjust `common_prefix_size()` to return `size_t`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-20T16:17:22Z","receivedAt":"2025-01-20T16:17:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `common_prefix_size()` function computes the length of the common\nprefix between two buffers. As such its return value will always be an\nunsigned integer, as the length cannot be negative. Regardless of that,\nthe function returns a signed integer, which is nonsensical and causes a\ncouple of -Wsign-compare warnings all over the place.\n\nAdjust the function to return a `size_t` instead.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/basics.c | 8 +++-----\n reftable/basics.h | 2 +-\n reftable/record.c | 4 ++--\n reftable/writer.c | 7 +++----\n t/unit-tests/t-reftable-basics.c | 2 +-\n 5 files changed, 10 insertions(+), 13 deletions(-)\n\ndiff --git a/reftable/basics.c b/reftable/basics.c\nindex fe2b83ff83..10b234ea55 100644\n--- a/reftable/basics.c\n+++ b/reftable/basics.c\n@@ -263,14 +263,12 @@ int names_equal(const char **a, const char **b)\n \treturn a[i] == b[i];\n }\n \n-int common_prefix_size(struct reftable_buf *a, struct reftable_buf *b)\n+size_t common_prefix_size(struct reftable_buf *a, struct reftable_buf *b)\n {\n-\tint p = 0;\n-\tfor (; p < a->len && p < b->len; p++) {\n+\tsize_t p = 0;\n+\tfor (; p < a->len && p < b->len; p++)\n \t\tif (a->buf[p] != b->buf[p])\n \t\t\tbreak;\n-\t}\n-\n \treturn p;\n }\n \ndiff --git a/reftable/basics.h b/reftable/basics.h\nindex 4bf71b0954..9ff81a68f8 100644\n--- a/reftable/basics.h\n+++ b/reftable/basics.h\n@@ -169,7 +169,7 @@ static inline void *reftable_alloc_grow(void *p, size_t nelem, size_t elsize,\n #endif\n \n /* Find the longest shared prefix size of `a` and `b` */\n-int common_prefix_size(struct reftable_buf *a, struct reftable_buf *b);\n+size_t common_prefix_size(struct reftable_buf *a, struct reftable_buf *b);\n \n int hash_size(enum reftable_hash id);\n \ndiff --git a/reftable/record.c b/reftable/record.c\nindex a55ce76aeb..4a3e019528 100644\n--- a/reftable/record.c\n+++ b/reftable/record.c\n@@ -144,9 +144,9 @@ int reftable_encode_key(int *restart, struct string_view dest,\n \t\t\tuint8_t extra)\n {\n \tstruct string_view start = dest;\n-\tint prefix_len = common_prefix_size(&prev_key, &key);\n+\tsize_t prefix_len = common_prefix_size(&prev_key, &key);\n \tuint64_t suffix_len = key.len - prefix_len;\n-\tint n = put_var_int(&dest, (uint64_t)prefix_len);\n+\tint n = put_var_int(&dest, prefix_len);\n \tif (n < 0)\n \t\treturn -1;\n \tstring_view_consume(&dest, n);\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex 740c98038e..91d6629486 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -577,7 +577,7 @@ static int writer_finish_section(struct reftable_writer *w)\n \n struct common_prefix_arg {\n \tstruct reftable_buf *last;\n-\tint max;\n+\tsize_t max;\n };\n \n static void update_common(void *void_arg, void *key)\n@@ -585,10 +585,9 @@ static void update_common(void *void_arg, void *key)\n \tstruct common_prefix_arg *arg = void_arg;\n \tstruct obj_index_tree_node *entry = key;\n \tif (arg->last) {\n-\t\tint n = common_prefix_size(&entry->hash, arg->last);\n-\t\tif (n > arg->max) {\n+\t\tsize_t n = common_prefix_size(&entry->hash, arg->last);\n+\t\tif (n > arg->max)\n \t\t\targ->max = n;\n-\t\t}\n \t}\n \targ->last = &entry->hash;\n }\ndiff --git a/t/unit-tests/t-reftable-basics.c b/t/unit-tests/t-reftable-basics.c\nindex 1d640b280f..9ba7eb05ad 100644\n--- a/t/unit-tests/t-reftable-basics.c\n+++ b/t/unit-tests/t-reftable-basics.c\n@@ -120,7 +120,7 @@ int cmd_main(int argc UNUSED, const char *argv[] UNUSED)\n \t\tfor (size_t i = 0; i < ARRAY_SIZE(cases); i++) {\n \t\t\tcheck(!reftable_buf_addstr(&a, cases[i].a));\n \t\t\tcheck(!reftable_buf_addstr(&b, cases[i].b));\n-\t\t\tcheck_int(common_prefix_size(&a, &b), ==, cases[i].want);\n+\t\t\tcheck_uint(common_prefix_size(&a, &b), ==, cases[i].want);\n \t\t\treftable_buf_reset(&a);\n \t\t\treftable_buf_reset(&b);\n \t\t}\n\n-- \n2.48.0.257.gd3603152ad.dirty\n\n"},{"id":"510961","messageId":"20250120-b4-pks-reftable-sign-compare-v2-5-b4566d02e4a5@pks.im","threadId":"62815","inReplyTo":"20250120-b4-pks-reftable-sign-compare-v2-0-b4566d02e4a5@pks.im","subject":"[PATCH v2 05/10] reftable/basics: adjust `hash_size()` to return `uint32_t`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-20T16:17:23Z","receivedAt":"2025-01-20T16:17:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `hash_size()` function returns the number of bytes used by the hash\nfunction. Weirdly enough though, it returns a signed integer for its\nsize even though the size obviously cannot ever be negative. The only\ncase where it could be negative is if the function returned an error\nwhen asked for an unknown hash, but we assert(3p) instead.\n\nAdjust the type of `hash_size()` to be `uint32_t` and adapt all places\nthat use signed integers for the hash size to follow suit. This also\nallows us to get rid of a couple asserts that we had which verified that\nthe size was indeed positive, which further stresses the point that this\nrefactoring makes sense.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/basics.c | 2 +-\n reftable/basics.h | 2 +-\n reftable/block.c | 4 ++--\n reftable/block.h | 10 ++++----\n reftable/reader.c | 2 +-\n reftable/record.c | 52 ++++++++++++++++++----------------------\n reftable/record.h | 16 ++++++-------\n reftable/reftable-record.h | 4 ++--\n t/unit-tests/t-reftable-record.c | 2 +-\n 9 files changed, 44 insertions(+), 50 deletions(-)\n\ndiff --git a/reftable/basics.c b/reftable/basics.c\nindex 10b234ea55..3b5ea27bbd 100644\n--- a/reftable/basics.c\n+++ b/reftable/basics.c\n@@ -272,7 +272,7 @@ size_t common_prefix_size(struct reftable_buf *a, struct reftable_buf *b)\n \treturn p;\n }\n \n-int hash_size(enum reftable_hash id)\n+uint32_t hash_size(enum reftable_hash id)\n {\n \tif (!id)\n \t\treturn REFTABLE_HASH_SIZE_SHA1;\ndiff --git a/reftable/basics.h b/reftable/basics.h\nindex 9ff81a68f8..a2a010a0e1 100644\n--- a/reftable/basics.h\n+++ b/reftable/basics.h\n@@ -171,7 +171,7 @@ static inline void *reftable_alloc_grow(void *p, size_t nelem, size_t elsize,\n /* Find the longest shared prefix size of `a` and `b` */\n size_t common_prefix_size(struct reftable_buf *a, struct reftable_buf *b);\n \n-int hash_size(enum reftable_hash id);\n+uint32_t hash_size(enum reftable_hash id);\n \n /*\n * Format IDs that identify the hash function used by a reftable. Note that\ndiff --git a/reftable/block.c b/reftable/block.c\nindex 9858bbc7c5..2380aabb2f 100644\n--- a/reftable/block.c\n+++ b/reftable/block.c\n@@ -72,7 +72,7 @@ static int block_writer_register_restart(struct block_writer *w, int n,\n }\n \n int block_writer_init(struct block_writer *bw, uint8_t typ, uint8_t *block,\n-\t\t uint32_t block_size, uint32_t header_off, int hash_size)\n+\t\t uint32_t block_size, uint32_t header_off, uint32_t hash_size)\n {\n \tbw->block = block;\n \tbw->hash_size = hash_size;\n@@ -214,7 +214,7 @@ int block_writer_finish(struct block_writer *w)\n \n int block_reader_init(struct block_reader *br, struct reftable_block *block,\n \t\t uint32_t header_off, uint32_t table_block_size,\n-\t\t int hash_size)\n+\t\t uint32_t hash_size)\n {\n \tuint32_t full_block_size = table_block_size;\n \tuint8_t typ = block->data[header_off];\ndiff --git a/reftable/block.h b/reftable/block.h\nindex 0431e8591f..5f67ed74c5 100644\n--- a/reftable/block.h\n+++ b/reftable/block.h\n@@ -30,7 +30,7 @@ struct block_writer {\n \n \t/* How often to restart keys. */\n \tuint16_t restart_interval;\n-\tint hash_size;\n+\tuint32_t hash_size;\n \n \t/* Offset of next uint8_t to write. */\n \tuint32_t next;\n@@ -48,7 +48,7 @@ struct block_writer {\n * initializes the blockwriter to write `typ` entries, using `block` as temporary\n * storage. `block` is not owned by the block_writer. */\n int block_writer_init(struct block_writer *bw, uint8_t typ, uint8_t *block,\n-\t\t uint32_t block_size, uint32_t header_off, int hash_size);\n+\t\t uint32_t block_size, uint32_t header_off, uint32_t hash_size);\n \n /* returns the block type (eg. 'r' for ref records. */\n uint8_t block_writer_type(struct block_writer *bw);\n@@ -72,7 +72,7 @@ struct block_reader {\n \n \t/* the memory block */\n \tstruct reftable_block block;\n-\tint hash_size;\n+\tuint32_t hash_size;\n \n \t/* Uncompressed data for log entries. */\n \tz_stream *zstream;\n@@ -92,7 +92,7 @@ struct block_reader {\n /* initializes a block reader. */\n int block_reader_init(struct block_reader *br, struct reftable_block *bl,\n \t\t uint32_t header_off, uint32_t table_block_size,\n-\t\t int hash_size);\n+\t\t uint32_t hash_size);\n \n void block_reader_release(struct block_reader *br);\n \n@@ -108,7 +108,7 @@ struct block_iter {\n \tuint32_t next_off;\n \tconst unsigned char *block;\n \tsize_t block_len;\n-\tint hash_size;\n+\tuint32_t hash_size;\n \n \t/* key for last entry we read. */\n \tstruct reftable_buf last_key;\ndiff --git a/reftable/reader.c b/reftable/reader.c\nindex ea82955c9b..9df8a5ecb1 100644\n--- a/reftable/reader.c\n+++ b/reftable/reader.c\n@@ -750,7 +750,7 @@ static int reftable_reader_refs_for_unindexed(struct reftable_reader *r,\n \tstruct table_iter *ti;\n \tstruct filtering_ref_iterator *filter = NULL;\n \tstruct filtering_ref_iterator empty = FILTERING_REF_ITERATOR_INIT;\n-\tint oid_len = hash_size(r->hash_id);\n+\tuint32_t oid_len = hash_size(r->hash_id);\n \tint err;\n \n \tREFTABLE_ALLOC_ARRAY(ti, 1);\ndiff --git a/reftable/record.c b/reftable/record.c\nindex 4a3e019528..f7766a32ef 100644\n--- a/reftable/record.c\n+++ b/reftable/record.c\n@@ -229,7 +229,7 @@ static int reftable_ref_record_key(const void *r, struct reftable_buf *dest)\n }\n \n static int reftable_ref_record_copy_from(void *rec, const void *src_rec,\n-\t\t\t\t\t int hash_size)\n+\t\t\t\t\t uint32_t hash_size)\n {\n \tstruct reftable_ref_record *ref = rec;\n \tconst struct reftable_ref_record *src = src_rec;\n@@ -237,8 +237,6 @@ static int reftable_ref_record_copy_from(void *rec, const void *src_rec,\n \tsize_t refname_cap = 0;\n \tint err;\n \n-\tassert(hash_size > 0);\n-\n \tSWAP(refname, ref->refname);\n \tSWAP(refname_cap, ref->refname_cap);\n \treftable_ref_record_release(ref);\n@@ -319,13 +317,12 @@ static uint8_t reftable_ref_record_val_type(const void *rec)\n }\n \n static int reftable_ref_record_encode(const void *rec, struct string_view s,\n-\t\t\t\t int hash_size)\n+\t\t\t\t uint32_t hash_size)\n {\n \tconst struct reftable_ref_record *r =\n \t\t(const struct reftable_ref_record *)rec;\n \tstruct string_view start = s;\n \tint n = put_var_int(&s, r->update_index);\n-\tassert(hash_size > 0);\n \tif (n < 0)\n \t\treturn -1;\n \tstring_view_consume(&s, n);\n@@ -365,7 +362,7 @@ static int reftable_ref_record_encode(const void *rec, struct string_view s,\n \n static int reftable_ref_record_decode(void *rec, struct reftable_buf key,\n \t\t\t\t uint8_t val_type, struct string_view in,\n-\t\t\t\t int hash_size, struct reftable_buf *scratch)\n+\t\t\t\t uint32_t hash_size, struct reftable_buf *scratch)\n {\n \tstruct reftable_ref_record *r = rec;\n \tstruct string_view start = in;\n@@ -374,8 +371,6 @@ static int reftable_ref_record_decode(void *rec, struct reftable_buf key,\n \tsize_t refname_cap = 0;\n \tint n, err;\n \n-\tassert(hash_size > 0);\n-\n \tn = get_var_int(&update_index, &in);\n \tif (n < 0)\n \t\treturn n;\n@@ -451,7 +446,7 @@ static int reftable_ref_record_is_deletion_void(const void *p)\n }\n \n static int reftable_ref_record_equal_void(const void *a,\n-\t\t\t\t\t const void *b, int hash_size)\n+\t\t\t\t\t const void *b, uint32_t hash_size)\n {\n \tstruct reftable_ref_record *ra = (struct reftable_ref_record *) a;\n \tstruct reftable_ref_record *rb = (struct reftable_ref_record *) b;\n@@ -495,7 +490,7 @@ static void reftable_obj_record_release(void *rec)\n }\n \n static int reftable_obj_record_copy_from(void *rec, const void *src_rec,\n-\t\t\t\t\t int hash_size UNUSED)\n+\t\t\t\t\t uint32_t hash_size UNUSED)\n {\n \tstruct reftable_obj_record *obj = rec;\n \tconst struct reftable_obj_record *src = src_rec;\n@@ -527,7 +522,7 @@ static uint8_t reftable_obj_record_val_type(const void *rec)\n }\n \n static int reftable_obj_record_encode(const void *rec, struct string_view s,\n-\t\t\t\t int hash_size UNUSED)\n+\t\t\t\t uint32_t hash_size UNUSED)\n {\n \tconst struct reftable_obj_record *r = rec;\n \tstruct string_view start = s;\n@@ -562,7 +557,7 @@ static int reftable_obj_record_encode(const void *rec, struct string_view s,\n \n static int reftable_obj_record_decode(void *rec, struct reftable_buf key,\n \t\t\t\t uint8_t val_type, struct string_view in,\n-\t\t\t\t int hash_size UNUSED,\n+\t\t\t\t uint32_t hash_size UNUSED,\n \t\t\t\t struct reftable_buf *scratch UNUSED)\n {\n \tstruct string_view start = in;\n@@ -626,7 +621,7 @@ static int not_a_deletion(const void *p UNUSED)\n }\n \n static int reftable_obj_record_equal_void(const void *a, const void *b,\n-\t\t\t\t\t int hash_size UNUSED)\n+\t\t\t\t\t uint32_t hash_size UNUSED)\n {\n \tstruct reftable_obj_record *ra = (struct reftable_obj_record *) a;\n \tstruct reftable_obj_record *rb = (struct reftable_obj_record *) b;\n@@ -701,7 +696,7 @@ static int reftable_log_record_key(const void *r, struct reftable_buf *dest)\n }\n \n static int reftable_log_record_copy_from(void *rec, const void *src_rec,\n-\t\t\t\t\t int hash_size)\n+\t\t\t\t\t uint32_t hash_size)\n {\n \tstruct reftable_log_record *dst = rec;\n \tconst struct reftable_log_record *src =\n@@ -782,7 +777,7 @@ static uint8_t reftable_log_record_val_type(const void *rec)\n }\n \n static int reftable_log_record_encode(const void *rec, struct string_view s,\n-\t\t\t\t int hash_size)\n+\t\t\t\t uint32_t hash_size)\n {\n \tconst struct reftable_log_record *r = rec;\n \tstruct string_view start = s;\n@@ -830,7 +825,7 @@ static int reftable_log_record_encode(const void *rec, struct string_view s,\n \n static int reftable_log_record_decode(void *rec, struct reftable_buf key,\n \t\t\t\t uint8_t val_type, struct string_view in,\n-\t\t\t\t int hash_size, struct reftable_buf *scratch)\n+\t\t\t\t uint32_t hash_size, struct reftable_buf *scratch)\n {\n \tstruct string_view start = in;\n \tstruct reftable_log_record *r = rec;\n@@ -978,7 +973,7 @@ static int null_streq(const char *a, const char *b)\n }\n \n static int reftable_log_record_equal_void(const void *a,\n-\t\t\t\t\t const void *b, int hash_size)\n+\t\t\t\t\t const void *b, uint32_t hash_size)\n {\n \treturn reftable_log_record_equal((struct reftable_log_record *) a,\n \t\t\t\t\t (struct reftable_log_record *) b,\n@@ -1002,7 +997,7 @@ static int reftable_log_record_cmp_void(const void *_a, const void *_b)\n }\n \n int reftable_log_record_equal(const struct reftable_log_record *a,\n-\t\t\t const struct reftable_log_record *b, int hash_size)\n+\t\t\t const struct reftable_log_record *b, uint32_t hash_size)\n {\n \tif (!(null_streq(a->refname, b->refname) &&\n \t a->update_index == b->update_index &&\n@@ -1056,7 +1051,7 @@ static int reftable_index_record_key(const void *r, struct reftable_buf *dest)\n }\n \n static int reftable_index_record_copy_from(void *rec, const void *src_rec,\n-\t\t\t\t\t int hash_size UNUSED)\n+\t\t\t\t\t uint32_t hash_size UNUSED)\n {\n \tstruct reftable_index_record *dst = rec;\n \tconst struct reftable_index_record *src = src_rec;\n@@ -1083,7 +1078,7 @@ static uint8_t reftable_index_record_val_type(const void *rec UNUSED)\n }\n \n static int reftable_index_record_encode(const void *rec, struct string_view out,\n-\t\t\t\t\tint hash_size UNUSED)\n+\t\t\t\t\tuint32_t hash_size UNUSED)\n {\n \tconst struct reftable_index_record *r =\n \t\t(const struct reftable_index_record *)rec;\n@@ -1101,7 +1096,7 @@ static int reftable_index_record_encode(const void *rec, struct string_view out,\n static int reftable_index_record_decode(void *rec, struct reftable_buf key,\n \t\t\t\t\tuint8_t val_type UNUSED,\n \t\t\t\t\tstruct string_view in,\n-\t\t\t\t\tint hash_size UNUSED,\n+\t\t\t\t\tuint32_t hash_size UNUSED,\n \t\t\t\t\tstruct reftable_buf *scratch UNUSED)\n {\n \tstruct string_view start = in;\n@@ -1122,7 +1117,7 @@ static int reftable_index_record_decode(void *rec, struct reftable_buf key,\n }\n \n static int reftable_index_record_equal(const void *a, const void *b,\n-\t\t\t\t int hash_size UNUSED)\n+\t\t\t\t uint32_t hash_size UNUSED)\n {\n \tstruct reftable_index_record *ia = (struct reftable_index_record *) a;\n \tstruct reftable_index_record *ib = (struct reftable_index_record *) b;\n@@ -1156,14 +1151,14 @@ int reftable_record_key(struct reftable_record *rec, struct reftable_buf *dest)\n }\n \n int reftable_record_encode(struct reftable_record *rec, struct string_view dest,\n-\t\t\t int hash_size)\n+\t\t\t uint32_t hash_size)\n {\n \treturn reftable_record_vtable(rec)->encode(reftable_record_data(rec),\n \t\t\t\t\t\t dest, hash_size);\n }\n \n int reftable_record_copy_from(struct reftable_record *rec,\n-\t\t\t struct reftable_record *src, int hash_size)\n+\t\t\t struct reftable_record *src, uint32_t hash_size)\n {\n \tassert(src->type == rec->type);\n \n@@ -1178,7 +1173,7 @@ uint8_t reftable_record_val_type(struct reftable_record *rec)\n }\n \n int reftable_record_decode(struct reftable_record *rec, struct reftable_buf key,\n-\t\t\t uint8_t extra, struct string_view src, int hash_size,\n+\t\t\t uint8_t extra, struct string_view src, uint32_t hash_size,\n \t\t\t struct reftable_buf *scratch)\n {\n \treturn reftable_record_vtable(rec)->decode(reftable_record_data(rec),\n@@ -1205,7 +1200,7 @@ int reftable_record_cmp(struct reftable_record *a, struct reftable_record *b)\n \t\treftable_record_data(a), reftable_record_data(b));\n }\n \n-int reftable_record_equal(struct reftable_record *a, struct reftable_record *b, int hash_size)\n+int reftable_record_equal(struct reftable_record *a, struct reftable_record *b, uint32_t hash_size)\n {\n \tif (a->type != b->type)\n \t\treturn 0;\n@@ -1213,7 +1208,7 @@ int reftable_record_equal(struct reftable_record *a, struct reftable_record *b,\n \t\treftable_record_data(a), reftable_record_data(b), hash_size);\n }\n \n-static int hash_equal(const unsigned char *a, const unsigned char *b, int hash_size)\n+static int hash_equal(const unsigned char *a, const unsigned char *b, uint32_t hash_size)\n {\n \tif (a && b)\n \t\treturn !memcmp(a, b, hash_size);\n@@ -1222,9 +1217,8 @@ static int hash_equal(const unsigned char *a, const unsigned char *b, int hash_s\n }\n \n int reftable_ref_record_equal(const struct reftable_ref_record *a,\n-\t\t\t const struct reftable_ref_record *b, int hash_size)\n+\t\t\t const struct reftable_ref_record *b, uint32_t hash_size)\n {\n-\tassert(hash_size > 0);\n \tif (!null_streq(a->refname, b->refname))\n \t\treturn 0;\n \ndiff --git a/reftable/record.h b/reftable/record.h\nindex 0df950f401..c7755a4d75 100644\n--- a/reftable/record.h\n+++ b/reftable/record.h\n@@ -47,18 +47,18 @@ struct reftable_record_vtable {\n \t/* The record type of ('r' for ref). */\n \tuint8_t type;\n \n-\tint (*copy_from)(void *dest, const void *src, int hash_size);\n+\tint (*copy_from)(void *dest, const void *src, uint32_t hash_size);\n \n \t/* a value of [0..7], indicating record subvariants (eg. ref vs. symref\n \t * vs ref deletion) */\n \tuint8_t (*val_type)(const void *rec);\n \n \t/* encodes rec into dest, returning how much space was used. */\n-\tint (*encode)(const void *rec, struct string_view dest, int hash_size);\n+\tint (*encode)(const void *rec, struct string_view dest, uint32_t hash_size);\n \n \t/* decode data from `src` into the record. */\n \tint (*decode)(void *rec, struct reftable_buf key, uint8_t extra,\n-\t\t struct string_view src, int hash_size,\n+\t\t struct string_view src, uint32_t hash_size,\n \t\t struct reftable_buf *scratch);\n \n \t/* deallocate and null the record. */\n@@ -68,7 +68,7 @@ struct reftable_record_vtable {\n \tint (*is_deletion)(const void *rec);\n \n \t/* Are two records equal? This assumes they have the same type. Returns 0 for non-equal. */\n-\tint (*equal)(const void *a, const void *b, int hash_size);\n+\tint (*equal)(const void *a, const void *b, uint32_t hash_size);\n \n \t/*\n \t * Compare keys of two records with each other. The records must have\n@@ -135,16 +135,16 @@ void reftable_record_init(struct reftable_record *rec, uint8_t typ);\n \n /* see struct record_vtable */\n int reftable_record_cmp(struct reftable_record *a, struct reftable_record *b);\n-int reftable_record_equal(struct reftable_record *a, struct reftable_record *b, int hash_size);\n+int reftable_record_equal(struct reftable_record *a, struct reftable_record *b, uint32_t hash_size);\n int reftable_record_key(struct reftable_record *rec, struct reftable_buf *dest);\n int reftable_record_copy_from(struct reftable_record *rec,\n-\t\t\t struct reftable_record *src, int hash_size);\n+\t\t\t struct reftable_record *src, uint32_t hash_size);\n uint8_t reftable_record_val_type(struct reftable_record *rec);\n int reftable_record_encode(struct reftable_record *rec, struct string_view dest,\n-\t\t\t int hash_size);\n+\t\t\t uint32_t hash_size);\n int reftable_record_decode(struct reftable_record *rec, struct reftable_buf key,\n \t\t\t uint8_t extra, struct string_view src,\n-\t\t\t int hash_size, struct reftable_buf *scratch);\n+\t\t\t uint32_t hash_size, struct reftable_buf *scratch);\n int reftable_record_is_deletion(struct reftable_record *rec);\n \n static inline uint8_t reftable_record_type(struct reftable_record *rec)\ndiff --git a/reftable/reftable-record.h b/reftable/reftable-record.h\nindex ddd48eb579..931e594744 100644\n--- a/reftable/reftable-record.h\n+++ b/reftable/reftable-record.h\n@@ -65,7 +65,7 @@ void reftable_ref_record_release(struct reftable_ref_record *ref);\n \n /* returns whether two reftable_ref_records are the same. Useful for testing. */\n int reftable_ref_record_equal(const struct reftable_ref_record *a,\n-\t\t\t const struct reftable_ref_record *b, int hash_size);\n+\t\t\t const struct reftable_ref_record *b, uint32_t hash_size);\n \n /* reftable_log_record holds a reflog entry */\n struct reftable_log_record {\n@@ -105,6 +105,6 @@ void reftable_log_record_release(struct reftable_log_record *log);\n \n /* returns whether two records are equal. Useful for testing. */\n int reftable_log_record_equal(const struct reftable_log_record *a,\n-\t\t\t const struct reftable_log_record *b, int hash_size);\n+\t\t\t const struct reftable_log_record *b, uint32_t hash_size);\n \n #endif\ndiff --git a/t/unit-tests/t-reftable-record.c b/t/unit-tests/t-reftable-record.c\nindex 6d912b9c8f..d49d2a2729 100644\n--- a/t/unit-tests/t-reftable-record.c\n+++ b/t/unit-tests/t-reftable-record.c\n@@ -76,7 +76,7 @@ static void t_varint_overflow(void)\n \n static void set_hash(uint8_t *h, int j)\n {\n-\tfor (int i = 0; i < hash_size(REFTABLE_HASH_SHA1); i++)\n+\tfor (size_t i = 0; i < hash_size(REFTABLE_HASH_SHA1); i++)\n \t\th[i] = (j >> i) & 0xff;\n }\n \n\n-- \n2.48.0.257.gd3603152ad.dirty\n\n"},{"id":"510964","messageId":"20250120-b4-pks-reftable-sign-compare-v2-6-b4566d02e4a5@pks.im","threadId":"62815","inReplyTo":"20250120-b4-pks-reftable-sign-compare-v2-0-b4566d02e4a5@pks.im","subject":"[PATCH v2 06/10] reftable/block: adapt header and footer size to return a `size_t`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-20T16:17:24Z","receivedAt":"2025-01-20T16:17:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The functions `header_size()` and `footer_size()` return a positive\ninteger representing the size of the header and footer, respectively,\ndependent on the version of the reftable format. Similar to the\npreceding commit, these functions return a signed integer though, which\nis nonsensical given that there is no way for these functions to return\nnegative.\n\nAdapt the functions to return a `size_t` instead to fix a couple of sign\ncomparison warnings.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/block.c | 4 ++--\n reftable/block.h | 4 ++--\n t/unit-tests/t-reftable-readwrite.c | 2 +-\n 3 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/reftable/block.c b/reftable/block.c\nindex 2380aabb2f..1275085257 100644\n--- a/reftable/block.c\n+++ b/reftable/block.c\n@@ -15,7 +15,7 @@ license that can be found in the LICENSE file or at\n #include \"system.h\"\n #include \n \n-int header_size(int version)\n+size_t header_size(int version)\n {\n \tswitch (version) {\n \tcase 1:\n@@ -26,7 +26,7 @@ int header_size(int version)\n \tabort();\n }\n \n-int footer_size(int version)\n+size_t footer_size(int version)\n {\n \tswitch (version) {\n \tcase 1:\ndiff --git a/reftable/block.h b/reftable/block.h\nindex 5f67ed74c5..bef2b8a4c5 100644\n--- a/reftable/block.h\n+++ b/reftable/block.h\n@@ -137,10 +137,10 @@ void block_iter_reset(struct block_iter *it);\n void block_iter_close(struct block_iter *it);\n \n /* size of file header, depending on format version */\n-int header_size(int version);\n+size_t header_size(int version);\n \n /* size of file footer, depending on format version */\n-int footer_size(int version);\n+size_t footer_size(int version);\n \n /* returns a block to its source. */\n void reftable_block_done(struct reftable_block *ret);\ndiff --git a/t/unit-tests/t-reftable-readwrite.c b/t/unit-tests/t-reftable-readwrite.c\nindex 6b75a419b9..2e553154ea 100644\n--- a/t/unit-tests/t-reftable-readwrite.c\n+++ b/t/unit-tests/t-reftable-readwrite.c\n@@ -643,7 +643,7 @@ static void t_write_empty_table(void)\n \tcheck_int(err, ==, REFTABLE_EMPTY_TABLE_ERROR);\n \treftable_writer_free(w);\n \n-\tcheck_int(buf.len, ==, header_size(1) + footer_size(1));\n+\tcheck_uint(buf.len, ==, header_size(1) + footer_size(1));\n \n \tblock_source_from_buf(&source, &buf);\n \n\n-- \n2.48.0.257.gd3603152ad.dirty\n\n"},{"id":"510962","messageId":"20250120-b4-pks-reftable-sign-compare-v2-7-b4566d02e4a5@pks.im","threadId":"62815","inReplyTo":"20250120-b4-pks-reftable-sign-compare-v2-0-b4566d02e4a5@pks.im","subject":"[PATCH v2 07/10] reftable/block: adjust type of the restart length","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-20T16:17:25Z","receivedAt":"2025-01-20T16:17:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The restart length is tracked as a positive integer even though it\ncannot ever be negative. Furthermore, it is effectively capped via the\nMAX_RESTARTS variable.\n\nAdjust the type of the variable to be `uint32_t`. While this type is\nexcessive given that MAX_RESTARTS fits into an `uint16_t`, other places\nalready use 32 bit integers for restarts, so this type is being more\nconsistent.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/block.c | 12 +++++-------\n reftable/reftable-writer.h | 2 +-\n 2 files changed, 6 insertions(+), 8 deletions(-)\n\ndiff --git a/reftable/block.c b/reftable/block.c\nindex 1275085257..8ac865ce78 100644\n--- a/reftable/block.c\n+++ b/reftable/block.c\n@@ -40,16 +40,15 @@ size_t footer_size(int version)\n static int block_writer_register_restart(struct block_writer *w, int n,\n \t\t\t\t\t int is_restart, struct reftable_buf *key)\n {\n-\tint rlen, err;\n+\tuint32_t rlen;\n+\tint err;\n \n \trlen = w->restart_len;\n-\tif (rlen >= MAX_RESTARTS) {\n+\tif (rlen >= MAX_RESTARTS)\n \t\tis_restart = 0;\n-\t}\n \n-\tif (is_restart) {\n+\tif (is_restart)\n \t\trlen++;\n-\t}\n \tif (2 + 3 * rlen + n > w->block_size - w->next)\n \t\treturn -1;\n \tif (is_restart) {\n@@ -148,8 +147,7 @@ int block_writer_add(struct block_writer *w, struct reftable_record *rec)\n \n int block_writer_finish(struct block_writer *w)\n {\n-\tint i;\n-\tfor (i = 0; i < w->restart_len; i++) {\n+\tfor (uint32_t i = 0; i < w->restart_len; i++) {\n \t\tput_be24(w->block + w->next, w->restarts[i]);\n \t\tw->next += 3;\n \t}\ndiff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h\nindex 5f9afa620b..bfef3b1721 100644\n--- a/reftable/reftable-writer.h\n+++ b/reftable/reftable-writer.h\n@@ -84,7 +84,7 @@ struct reftable_block_stats {\n \t/* total number of entries written */\n \tint entries;\n \t/* total number of key restarts */\n-\tint restarts;\n+\tuint32_t restarts;\n \t/* total number of blocks */\n \tint blocks;\n \t/* total number of index blocks */\n\n-- \n2.48.0.257.gd3603152ad.dirty\n\n"},{"id":"510963","messageId":"20250120-b4-pks-reftable-sign-compare-v2-8-b4566d02e4a5@pks.im","threadId":"62815","inReplyTo":"20250120-b4-pks-reftable-sign-compare-v2-0-b4566d02e4a5@pks.im","subject":"[PATCH v2 08/10] reftable/blocksource: adjust type of the block length","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-20T16:17:26Z","receivedAt":"2025-01-20T16:17:33Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The block length is used to track the number of bytes available in a\nspecific block. As such, it is never set to a negative value, but is\nstill represented by a signed integer.\n\nAdjust the type of the variable to be `size_t`.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/reftable-blocksource.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/reftable/reftable-blocksource.h b/reftable/reftable-blocksource.h\nindex 5aa3990a57..f06ad52e0a 100644\n--- a/reftable/reftable-blocksource.h\n+++ b/reftable/reftable-blocksource.h\n@@ -22,7 +22,7 @@ struct reftable_block_source {\n * so it can return itself into the pool. */\n struct reftable_block {\n \tuint8_t *data;\n-\tint len;\n+\tsize_t len;\n \tstruct reftable_block_source source;\n };\n \n\n-- \n2.48.0.257.gd3603152ad.dirty\n\n"},{"id":"510965","messageId":"20250120-b4-pks-reftable-sign-compare-v2-9-b4566d02e4a5@pks.im","threadId":"62815","inReplyTo":"20250120-b4-pks-reftable-sign-compare-v2-0-b4566d02e4a5@pks.im","subject":"[PATCH v2 09/10] reftable/blocksource: adjust `read_block()` to return `ssize_t`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-20T16:17:27Z","receivedAt":"2025-01-20T16:17:33Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `block_source_read_block()` function and its implementations return\nan integer as a result that reflects either the number of bytes read, or\nan error. As such its return type, a signed integer, isn't wrong, but it\ndoesn't give the reader a good hint what it actually returns.\n\nRefactor the function to return an `ssize_t` instead, which is typical\nfor functions similar to read(3p) and should thus give readers a better\nsignal what they can expect as a result.\n\nAdjust callers to better handle the returned value to avoid warnings\nwith -Wsign-compare. One of these callers is `reader_get_block()`, whose\nreturn value is only ever used by its callers to figure out whether or\nnot the read was successful. So instead of bubbling up the `ssize_t`\nthere, too, we adapt it to only indicate success or errors.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/blocksource.c | 8 ++++----\n reftable/reader.c | 30 +++++++++++++++++-------------\n reftable/reader.h | 6 +++---\n reftable/reftable-blocksource.h | 11 +++++++----\n 4 files changed, 31 insertions(+), 24 deletions(-)\n\ndiff --git a/reftable/blocksource.c b/reftable/blocksource.c\nindex 52e0915a67..bba4a45b98 100644\n--- a/reftable/blocksource.c\n+++ b/reftable/blocksource.c\n@@ -24,8 +24,8 @@ static void reftable_buf_close(void *b UNUSED)\n {\n }\n \n-static int reftable_buf_read_block(void *v, struct reftable_block *dest,\n-\t\t\t\t uint64_t off, uint32_t size)\n+static ssize_t reftable_buf_read_block(void *v, struct reftable_block *dest,\n+\t\t\t\t uint64_t off, uint32_t size)\n {\n \tstruct reftable_buf *b = v;\n \tassert(off + size <= b->len);\n@@ -78,8 +78,8 @@ static void file_close(void *v)\n \treftable_free(b);\n }\n \n-static int file_read_block(void *v, struct reftable_block *dest, uint64_t off,\n-\t\t\t uint32_t size)\n+static ssize_t file_read_block(void *v, struct reftable_block *dest, uint64_t off,\n+\t\t\t uint32_t size)\n {\n \tstruct file_block_source *b = v;\n \tassert(off + size <= b->size);\ndiff --git a/reftable/reader.c b/reftable/reader.c\nindex 9df8a5ecb1..3f2e4b2800 100644\n--- a/reftable/reader.c\n+++ b/reftable/reader.c\n@@ -20,11 +20,11 @@ uint64_t block_source_size(struct reftable_block_source *source)\n \treturn source->ops->size(source->arg);\n }\n \n-int block_source_read_block(struct reftable_block_source *source,\n-\t\t\t struct reftable_block *dest, uint64_t off,\n-\t\t\t uint32_t size)\n+ssize_t block_source_read_block(struct reftable_block_source *source,\n+\t\t\t\tstruct reftable_block *dest, uint64_t off,\n+\t\t\t\tuint32_t size)\n {\n-\tint result = source->ops->read_block(source->arg, dest, off, size);\n+\tssize_t result = source->ops->read_block(source->arg, dest, off, size);\n \tdest->source = *source;\n \treturn result;\n }\n@@ -57,14 +57,17 @@ static int reader_get_block(struct reftable_reader *r,\n \t\t\t struct reftable_block *dest, uint64_t off,\n \t\t\t uint32_t sz)\n {\n+\tssize_t bytes_read;\n \tif (off >= r->size)\n \t\treturn 0;\n-\n-\tif (off + sz > r->size) {\n+\tif (off + sz > r->size)\n \t\tsz = r->size - off;\n-\t}\n \n-\treturn block_source_read_block(&r->source, dest, off, sz);\n+\tbytes_read = block_source_read_block(&r->source, dest, off, sz);\n+\tif (bytes_read < 0)\n+\t\treturn (int)bytes_read;\n+\n+\treturn 0;\n }\n \n enum reftable_hash reftable_reader_hash_id(struct reftable_reader *r)\n@@ -601,6 +604,7 @@ int reftable_reader_new(struct reftable_reader **out,\n \tstruct reftable_reader *r;\n \tuint64_t file_size = block_source_size(source);\n \tuint32_t read_size;\n+\tssize_t bytes_read;\n \tint err;\n \n \tREFTABLE_CALLOC_ARRAY(r, 1);\n@@ -619,8 +623,8 @@ int reftable_reader_new(struct reftable_reader **out,\n \t\tgoto done;\n \t}\n \n-\terr = block_source_read_block(source, &header, 0, read_size);\n-\tif (err != read_size) {\n+\tbytes_read = block_source_read_block(source, &header, 0, read_size);\n+\tif (bytes_read < 0 || (size_t)bytes_read != read_size) {\n \t\terr = REFTABLE_IO_ERROR;\n \t\tgoto done;\n \t}\n@@ -645,9 +649,9 @@ int reftable_reader_new(struct reftable_reader **out,\n \tr->hash_id = 0;\n \tr->refcount = 1;\n \n-\terr = block_source_read_block(source, &footer, r->size,\n-\t\t\t\t footer_size(r->version));\n-\tif (err != footer_size(r->version)) {\n+\tbytes_read = block_source_read_block(source, &footer, r->size,\n+\t\t\t\t\t footer_size(r->version));\n+\tif (bytes_read < 0 || (size_t)bytes_read != footer_size(r->version)) {\n \t\terr = REFTABLE_IO_ERROR;\n \t\tgoto done;\n \t}\ndiff --git a/reftable/reader.h b/reftable/reader.h\nindex d2b48a4849..bb72108a6f 100644\n--- a/reftable/reader.h\n+++ b/reftable/reader.h\n@@ -16,9 +16,9 @@ license that can be found in the LICENSE file or at\n \n uint64_t block_source_size(struct reftable_block_source *source);\n \n-int block_source_read_block(struct reftable_block_source *source,\n-\t\t\t struct reftable_block *dest, uint64_t off,\n-\t\t\t uint32_t size);\n+ssize_t block_source_read_block(struct reftable_block_source *source,\n+\t\t\t\tstruct reftable_block *dest, uint64_t off,\n+\t\t\t\tuint32_t size);\n void block_source_close(struct reftable_block_source *source);\n \n /* metadata for a block type */\ndiff --git a/reftable/reftable-blocksource.h b/reftable/reftable-blocksource.h\nindex f06ad52e0a..6b326aa5ea 100644\n--- a/reftable/reftable-blocksource.h\n+++ b/reftable/reftable-blocksource.h\n@@ -31,10 +31,13 @@ struct reftable_block_source_vtable {\n \t/* returns the size of a block source */\n \tuint64_t (*size)(void *source);\n \n-\t/* reads a segment from the block source. It is an error to read\n-\t beyond the end of the block */\n-\tint (*read_block)(void *source, struct reftable_block *dest,\n-\t\t\t uint64_t off, uint32_t size);\n+\t/*\n+\t * Reads a segment from the block source. It is an error to read beyond\n+\t * the end of the block.\n+\t */\n+\tssize_t (*read_block)(void *source, struct reftable_block *dest,\n+\t\t\t uint64_t off, uint32_t size);\n+\n \t/* mark the block as read; may return the data back to malloc */\n \tvoid (*return_block)(void *source, struct reftable_block *blockp);\n \n\n-- \n2.48.0.257.gd3603152ad.dirty\n\n"},{"id":"510966","messageId":"20250120-b4-pks-reftable-sign-compare-v2-10-b4566d02e4a5@pks.im","threadId":"62815","inReplyTo":"20250120-b4-pks-reftable-sign-compare-v2-0-b4566d02e4a5@pks.im","subject":"[PATCH v2 10/10] reftable: address trivial -Wsign-compare warnings","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-20T16:17:28Z","receivedAt":"2025-01-20T16:17:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Address the last couple of trivial -Wsign-compare warnings in the\nreftable library and remove the DISABLE_SIGN_COMPARE_WARNINGS macro that\nwe have in \"reftable/system.h\".\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/record.c | 7 ++-----\n reftable/stack.c | 12 +++++-------\n reftable/system.h | 2 --\n 3 files changed, 7 insertions(+), 14 deletions(-)\n\ndiff --git a/reftable/record.c b/reftable/record.c\nindex f7766a32ef..8919df8a4d 100644\n--- a/reftable/record.c\n+++ b/reftable/record.c\n@@ -126,7 +126,7 @@ static int decode_string(struct reftable_buf *dest, struct string_view in)\n static int encode_string(const char *str, struct string_view s)\n {\n \tstruct string_view start = s;\n-\tint l = strlen(str);\n+\tsize_t l = strlen(str);\n \tint n = put_var_int(&s, l);\n \tif (n < 0)\n \t\treturn -1;\n@@ -565,7 +565,6 @@ static int reftable_obj_record_decode(void *rec, struct reftable_buf key,\n \tuint64_t count = val_type;\n \tint n = 0;\n \tuint64_t last;\n-\tint j;\n \n \treftable_obj_record_release(r);\n \n@@ -600,8 +599,7 @@ static int reftable_obj_record_decode(void *rec, struct reftable_buf key,\n \tstring_view_consume(&in, n);\n \n \tlast = r->offsets[0];\n-\tj = 1;\n-\twhile (j < count) {\n+\tfor (uint64_t j = 1; j < count; j++) {\n \t\tuint64_t delta = 0;\n \t\tint n = get_var_int(&delta, &in);\n \t\tif (n < 0) {\n@@ -610,7 +608,6 @@ static int reftable_obj_record_decode(void *rec, struct reftable_buf key,\n \t\tstring_view_consume(&in, n);\n \n \t\tlast = r->offsets[j] = (delta + last);\n-\t\tj++;\n \t}\n \treturn start.len - in.len;\n }\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 531660a49f..5c0d6273a7 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -220,9 +220,9 @@ void reftable_stack_destroy(struct reftable_stack *st)\n \t}\n \n \tif (st->readers) {\n-\t\tint i = 0;\n \t\tstruct reftable_buf filename = REFTABLE_BUF_INIT;\n-\t\tfor (i = 0; i < st->readers_len; i++) {\n+\n+\t\tfor (size_t i = 0; i < st->readers_len; i++) {\n \t\t\tconst char *name = reader_name(st->readers[i]);\n \t\t\tint try_unlinking = 1;\n \n@@ -238,6 +238,7 @@ void reftable_stack_destroy(struct reftable_stack *st)\n \t\t\t\tunlink(filename.buf);\n \t\t\t}\n \t\t}\n+\n \t\treftable_buf_release(&filename);\n \t\tst->readers_len = 0;\n \t\tREFTABLE_FREE_AND_NULL(st->readers);\n@@ -568,7 +569,6 @@ static int stack_uptodate(struct reftable_stack *st)\n {\n \tchar **names = NULL;\n \tint err;\n-\tint i = 0;\n \n \t/*\n \t * When we have cached stat information available then we use it to\n@@ -608,7 +608,7 @@ static int stack_uptodate(struct reftable_stack *st)\n \tif (err < 0)\n \t\treturn err;\n \n-\tfor (i = 0; i < st->readers_len; i++) {\n+\tfor (size_t i = 0; i < st->readers_len; i++) {\n \t\tif (!names[i]) {\n \t\t\terr = 1;\n \t\t\tgoto done;\n@@ -1767,14 +1767,12 @@ static int reftable_stack_clean_locked(struct reftable_stack *st)\n \t}\n \n \twhile ((d = readdir(dir))) {\n-\t\tint i = 0;\n \t\tint found = 0;\n \t\tif (!is_table_name(d->d_name))\n \t\t\tcontinue;\n \n-\t\tfor (i = 0; !found && i < st->readers_len; i++) {\n+\t\tfor (size_t i = 0; !found && i < st->readers_len; i++)\n \t\t\tfound = !strcmp(reader_name(st->readers[i]), d->d_name);\n-\t\t}\n \t\tif (found)\n \t\t\tcontinue;\n \ndiff --git a/reftable/system.h b/reftable/system.h\nindex 5274eca1d0..7d5f803eeb 100644\n--- a/reftable/system.h\n+++ b/reftable/system.h\n@@ -11,8 +11,6 @@ license that can be found in the LICENSE file or at\n \n /* This header glues the reftable library to the rest of Git */\n \n-#define DISABLE_SIGN_COMPARE_WARNINGS\n-\n #include \"git-compat-util.h\"\n \n /*\n\n-- \n2.48.0.257.gd3603152ad.dirty\n\n"}]}