Re: [PATCH v3 6/8] refs: implement partial reference transaction support

From: Jeff King <peff@peff.net>
Date: Mar 7, 2025, 19:57 UTC
Message-ID: <20250307195740.GA3675378@coredump.intra.peff.net>
In-Reply-To: <20250305-245-partially-atomic-ref-updates-v3-6-0c64e3052354@gmail.com>

On Wed, Mar 05, 2025 at 06:39:01PM +0100, Karthik Nayak wrote:

Show 21 quoted lines

> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index 0132b8b06a..dd9912d637 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -1371,8 +1371,15 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
>  					    transaction->updates[i],
>  					    &refnames_to_check, head_type,
>  					    &head_referent, &referent, err);
> -		if (ret)
> +		if (ret) {
> +			if (ref_transaction_maybe_set_rejected(transaction, i, ret)) {
> +				strbuf_setlen(err, 0);
> +				ret = 0;
> +
> +				continue;
> +			}
>  			goto done;
> +		}
>  	}
>  
>  	string_list_sort(&refnames_to_check);

Coverity complains that this "ret = 0" is a dead store. I think it's right, because either:

  1. Our continue loops again, and we overwrite "ret" with the next call
     to prepare_single_update().

  2. We leave the loop (because this is the final entry in the
     transaction update array), and then we overwrite "ret" with the
     result of refs_verify_refnames_available().

But it may be better to leave it in place as a defensive measure against the rest of the function changing.

-Peff