On Wed, Mar 12, 2025 at 05:41:48PM +0530, Meet Soni wrote:
> diff --git a/reftable/writer.c b/reftable/writer.c
> index f3ab1035d6..0d8181e227 100644
> --- a/reftable/writer.c
> +++ b/reftable/writer.c
> @@ -310,11 +310,12 @@ static int writer_add_record(struct reftable_writer *w,
> * done. Otherwise the block writer may have hit the block size limit
> * and needs to be flushed.
> */
> - if (!block_writer_add(w->block_writer, rec)) {
> - err = 0;
> + err = block_writer_add(w->block_writer, rec);
> + if (err == 0)
> goto done;
> - }
Style: we'd typically say `if (!err)` here, even though I see that we
have explicit comparisons with 0 elsewhere in this file, too. So I guess
ultimately this is okay.
> @@ -327,18 +328,11 @@ static int writer_add_record(struct reftable_writer *w,
> goto done;
>
> /*
> - * Try to add the record to the writer again. If this still fails then
> - * the record does not fit into the block size.
> - *
> - * TODO: it would be great to have `block_writer_add()` return proper
> - * error codes so that we don't have to second-guess the failure
> - * mode here.
> + * Try to add the record to the writer again.
> */
My comment on the preceding version still applies here: the second
sentence (the one starting with "If this still fails...") should be
retained.
> err = block_writer_add(w->block_writer, rec);
> - if (err) {
> - err = REFTABLE_ENTRY_TOO_BIG_ERROR;
> + if (err)
> goto done;
> - }
>
> done:
> return err;
> @@ -625,10 +619,22 @@ static void write_object_record(void *void_arg, void *key)
> if (arg->err < 0)
> goto done;
>
> + /*
> + * Try to add the record to the writer. If this succeeds then we're
> + * done. Otherwise the block writer may have hit the block size limit
> + * and needs to be flushed.
> + */
> arg->err = block_writer_add(arg->w->block_writer, &rec);
> if (arg->err == 0)
> goto done;
>
> + if (arg->err != REFTABLE_ENTRY_TOO_BIG_ERROR)
> + goto done;
Good catch that there is another such pattern!
> + /*
> + * The current block is full, so we need to flush and reinitialize the
> + * writer to start writing the next block.
> + */
> arg->err = writer_flush_block(arg->w);
> if (arg->err < 0)
> goto done;
But there is another case further down where we do `block_writer_add()`
and then re-try in case the write fails. This one is a bit more curious:
if the write fails, we don't create a new block -- after all we have
just created one. Instead, we reset the record's offset length to zero
before retrying.
I _think_ that this is done because we know that when resetting the
offset we would write less data to the block, as can be seen in
`reftable_obj_record_encode()`. But I'm honestly not quite sure here as
I haven't yet done a deep dive into object records -- after all, we
don't even really use them in Git.
In any case, I think that this callsite also needs adjustment and
warrants a comment. And if so, all changes to `write_object_record()`
should probably go into a separate commit, as well.
Thanks!
Patrick
