Re: [GSoC PATCH] reftable: return proper error code from block_writer_add()

[Junio C Hamano <gitster@pobox.com>]

Meet Soni <meetsoni3017@gmail.com> writes:

> Previously, block_writer_add() used to return generic -1, which forced
> an assumption about the error type.
>
> Replace generic -1 returns in block_writer_add() and related functions
> with defined error codes.

What's missing from this proposed log message is an audit of the
callers to tell readers that this change is safe and expected by the
callers.  IOW, are there callers that start to behave differently
when they see ENTRY_TOO_BIG instead of -1, for example?

> Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
> ---
> This patch attempts to avoid making an assumption regarding error codes
> returned by block_writer_add().
>  reftable/block.c  |  9 +++++----
>  reftable/record.c | 16 +++++++++++-----
>  reftable/writer.c |  8 +-------
>  3 files changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/reftable/block.c b/reftable/block.c
> index b14a8f1259..50fbac801a 100644
> --- a/reftable/block.c
> +++ b/reftable/block.c
> @@ -49,7 +49,7 @@ static int block_writer_register_restart(struct block_writer *w, int n,
>  	if (is_restart)
>  		rlen++;
>  	if (2 + 3 * rlen + n > w->block_size - w->next)
> -		return -1;
> +		return REFTABLE_ENTRY_TOO_BIG_ERROR;

So this makes block_writer_register_restart() to return -11 instead
of -1; the sole caller of the function is block_writer_add() that
begins like so:

        /* Adds the reftable_record to the block. Returns -1 if it does not fit, 0 on
           success. Returns REFTABLE_API_ERROR if attempting to write a record with
           empty key. */
        int block_writer_add(struct block_writer *w, struct reftable_record *rec)
        {

Needless to say, the comment before the function needs to be
adjusted together with the above hunk (and others).  But more
importantly, are existing callers of this function now expected to
adjust to the change in behaviour when they receive the return value
of this function?  It used to be sufficient for them to deal with
-1, 0 or API_ERROR, but now they are required to handle other errors
(like the one that comes back from reftable_encode_key().  Do they
already handle these new error codes just fine?  Have you traced the
code paths to see how they react to them?

> @@ -115,8 +115,9 @@ int block_writer_add(struct block_writer *w, struct reftable_record *rec)
>  	int err;
>  
>  	err = reftable_record_key(rec, &w->scratch);
> -	if (err < 0)
> +	if (err < 0) {
>  		goto done;
> +	}

This is unwarranted, isn't it?

> @@ -126,14 +127,14 @@ int block_writer_add(struct block_writer *w, struct reftable_record *rec)
>  	n = reftable_encode_key(&is_restart, out, last, w->scratch,
>  				reftable_record_val_type(rec));
>  	if (n < 0) {
> -		err = -1;
> +		err = n;
>  		goto done;
>  	}

Here, block_writer_add() starts returning new error codes to its
callers that it never returned.

>  	string_view_consume(&out, n);
>  
>  	n = reftable_record_encode(rec, out, w->hash_size);
>  	if (n < 0) {
> -		err = -1;
> +		err = n;
>  		goto done;
>  	}

Ditto.

Note that I am not saying that it is a bad idea to make the error
codes more specific so that the callers can tell them apart.  I am
only saying that the patch that makes such a change must also make
sure that the callers are prepared to handle error coes that they
have never seen from the current callee.

The same applies to the remainder of the patch.

Thanks.