Re: [PATCH v3 2/2] reftable: adapt writer code to propagate block_writer_add() errors

From: Patrick Steinhardt <ps@pks.im>
Date: Mar 12, 2025, 12:49 UTC
Message-ID: <Z9GC400L-XV3SFyj@pks.im>
In-Reply-To: <20250312121148.1879604-3-meetsoni3017@gmail.com>

On Wed, Mar 12, 2025 at 05:41:48PM +0530, Meet Soni wrote:

Show 14 quoted lines

> 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.

Show 12 quoted lines

> @@ -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.

Show 24 quoted lines

>  	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!

Show 7 quoted lines

> +	/*
> +	 * 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