Re: [PATCH v2 6/7] refs: implement partial reference transaction support

From: Patrick Steinhardt <ps@pks.im>
Date: Feb 25, 2025, 11:07 UTC
Message-ID: <Z72kjTRDNOM6iFkY@pks.im>
In-Reply-To: <20250225-245-partially-atomic-ref-updates-v2-6-cfa3236895d7@gmail.com>

On Tue, Feb 25, 2025 at 10:29:09AM +0100, Karthik Nayak wrote:

Show 20 quoted lines

> diff --git a/refs.c b/refs.c
> index f989a46a5a..243c09c368 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2726,6 +2736,27 @@ void ref_transaction_for_each_queued_update(struct ref_transaction *transaction,
>  	}
>  }
>  
> +void ref_transaction_for_each_rejected_update(struct ref_transaction *transaction,
> +					      ref_transaction_for_each_rejected_update_fn cb,
> +					      void *cb_data)
> +{
> +	if (!(transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL))
> +		return;
> +
> +	for (size_t i = 0; i < transaction->nr; i++) {
> +		struct ref_update *update = transaction->updates[i];
> +
> +		if (!update->rejection_err)
> +			continue;

This kind of proves my point that `TRANSACTION_OK` is pointless and leads to a mixture of using and not using the enum :)

Show 11 quoted lines

> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 3b0adf8bb2..d0a53c9ace 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2851,8 +2851,18 @@ static int files_transaction_prepare(struct ref_store *ref_store,
>  		ret = lock_ref_for_update(refs, update, transaction,
>  					  head_ref, &refnames_to_check,
>  					  err);
> -		if (ret)
> +		if (ret) {
> +			if (transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL &&

Hm. If the error values were defined as a bitfield we could refactor this to not be a flag, but have a `transaction->accepted_rejections` instead that allows the caller to ask for only a subset of rejections to be accepted.

I'm not quite sure whether it is a good idea, but the logic to handle all of that could be self-contained in `ref_transaction_set_rejected()`: if it returned an error code itself, it would swallow any errors in case the transaction allows a given error, and bubble up the error again in case the error is not allowed. The function could use a rename in that case though, e.g. `ref_transaction_maybe_set_rejected()`.

Patrick