"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> diff --git a/reftable/reader.c b/reftable/reader.c
> index 006709a645a..0d16b098f5e 100644
> --- a/reftable/reader.c
> +++ b/reftable/reader.c
> @@ -290,28 +290,34 @@ int reader_init_block_reader(struct reftable_reader *r, struct block_reader *br,
>
> err = reader_get_block(r, &block, next_off, guess_block_size);
> if (err < 0)
> - return err;
> + goto done;
>
> block_size = extract_block_size(block.data, &block_typ, next_off,
> r->version);
> - if (block_size < 0)
> - return block_size;
> -
> + if (block_size < 0) {
> + err = block_size;
> + goto done;
> + }
> if (want_typ != BLOCK_TYPE_ANY && block_typ != want_typ) {
> - reftable_block_done(&block);
> - return 1;
> + err = 1;
> + goto done;
> }
>
> if (block_size > guess_block_size) {
> reftable_block_done(&block);
> err = reader_get_block(r, &block, next_off, block_size);
> if (err < 0) {
> - return err;
> + goto done;
> }
> }
>
> - return block_reader_init(br, &block, header_off, r->block_size,
> - hash_size(r->hash_id));
> + err = block_reader_init(br, &block, header_off, r->block_size,
> + hash_size(r->hash_id));
> +done:
> + if (err)
Is the convention for reader_init() different from all other
functions? It makes reader wonder why this is not
if (err < 0)
even though it is not wrong per-se (as long as "zero means success"
is a part of the return value convention).
> + reftable_block_done(&block);
> +
> + return err;
> }
This one is new in this round. All look good, other than that one
check for error return.
> diff --git a/reftable/readwrite_test.c b/reftable/readwrite_test.c
> index 5f6bcc2f775..6e88182a83a 100644
> --- a/reftable/readwrite_test.c
> +++ b/reftable/readwrite_test.c
> @@ -254,6 +254,71 @@ static void test_log_write_read(void)
> reader_close(&rd);
> }
>
> +static void test_log_zlib_corruption(void)
> +{
> + struct reftable_write_options opts = {
> + .block_size = 256,
> + };
> + struct reftable_iterator it = { 0 };
> + struct reftable_reader rd = { 0 };
> + struct reftable_block_source source = { 0 };
> + struct strbuf buf = STRBUF_INIT;
> + struct reftable_writer *w =
> + reftable_new_writer(&strbuf_add_void, &buf, &opts);
> + const struct reftable_stats *stats = NULL;
> + uint8_t hash1[GIT_SHA1_RAWSZ] = { 1 };
> + uint8_t hash2[GIT_SHA1_RAWSZ] = { 2 };
Will this code be exercised when compiling with SHA256 support? If
not, this is perfectly fine, but otherwise, this needs to be MAX,
not SHA1, no?
> + char message[100] = { 0 };
You're filling this to the sizeof(message)-1, so we can afford to
leave it uninitialized.
> + int err, i, n;
> +
> + struct reftable_log_record log = {
> + .refname = "refname",
> + .value_type = REFTABLE_LOG_UPDATE,
> + .value = {
> + .update = {
> + .new_hash = hash1,
> + .old_hash = hash2,
> + .name = "My Name",
> + .email = "myname@invalid",
> + .message = message,
> + },
> + },
> + };
> +
> + for (i = 0; i < sizeof(message)-1; i++)
Style: SP around "-" on both sides.
> + message[i] = (uint8_t)(rand() % 64 + ' ');
> +
> + reftable_writer_set_limits(w, 1, 1);
