On Wed, Dec 22, 2021 at 11:51 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
> > + if (err)
>
> Is the convention for reader_init() different from all other
> functions? It makes reader wonder why this is not
>
> if (err < 0)
> > + reftable_block_done(&block);
>
> even though it is not wrong per-se (as long as "zero means success"
> is a part of the return value convention).
err > 0 is returned when we reach the end of the iteration, and this
function can generate err==1.
Normally, block_reader_init() transfers the block to the block_reader.
If err > 0, we skip that, so we'd be leaking the block.
At the same time, this means the "if (err)" is superfluous. In the
success case, the block was transferred to the block_reader, so the
reftable_block_done() call is a nop.
>
> > +
> > + 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?
The code is parameterized in terms of hash_size, so we don't have to
test both flavors exhaustively. There is a
test_table_read_write_seek_linear_sha256() that ensures that the basic
functionality works for SHA256.
> > + char message[100] = { 0 };
>
> You're filling this to the sizeof(message)-1, so we can afford to
> leave it uninitialized.
At the same time, we can afford to initialize it :-)
I'd rather not think about this, and always initialize everything.
> > + for (i = 0; i < sizeof(message)-1; i++)
>
> Style: SP around "-" on both sides.
done.
(I assume I don't have to resend the whole series for these small
tweaks? I'll wait if anyone else has comments, and send a reroll early
January)
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
